fix buggy locking for migration path

[matthyx]

Summary by CodeRabbit

• Bug Fixes

  • Fixed locking behavior during container profile retrieval to prevent race conditions and ensure consistent reads during concurrent access.

• Performance

  • Improved concurrent access and migration handling to reduce contention and speed up storage read/write operations under load.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28e33fc1-027b-40fe-a873-7d733e4cb8f7

📥 Commits

Reviewing files that changed from the base of the PR and between 3f4b902 and 098d253.

📒 Files selected for processing (2)

• pkg/registry/file/containerprofile_storage.go
• pkg/registry/file/storage.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/registry/file/containerprofile_storage.go
• pkg/registry/file/storage.go

---

📝 Walkthrough

Walkthrough

Update introduces a lock-state parameter for internal retrieval, changes migration locking behavior, and extracts migration steps into a migrateObject helper; callers were adjusted to pass the appropriate lock context.

Changes

Cohort / File(s)	Summary
Storage internals pkg/registry/file/storage.go	Added getLockState enum (noLock, hasReadLock, hasWriteLock); extended internal get to accept lockState; reworked gob type-mismatch migration flow to handle lock transitions per state; added migrateObject helper for migration → decode → rewrite sequence.
Caller adaptation pkg/registry/file/containerprofile_storage.go	Updated GetTsContainerProfile to call get with the noLock argument to reflect the new lockState parameter.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Caller as Client/Caller
participant Storage as FileStorage.get(...)
participant Migrator as /usr/bin/migration
participant FS as Filesystem/saveObject
Caller->>Storage: get(key, lockState)
alt object needs migration
Storage->>Storage: acquire/release locks based on lockState
Storage->>Migrator: run migration (stdin: gob/old data)
Migrator-->>Storage: JSON output (migrated object)
Storage->>FS: saveObject(migrated)
FS-->>Storage: success
end
Storage-->>Caller: return object, err

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nibbled at locks, three ways to chew,
NoLock, ReadLock, Write—each with its cue.
I hopped through migration, tidy and fleet,
Rewritten bytes tucked in burrowed sleet.
A rabbit's small patchwork — now snug and neat. ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix buggy locking for migration path' directly and specifically describes the main change: addressing locking bugs in the migration code path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-migration

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/registry/file/storage.go`:
- Around line 501-509: The deferred unchecked call to s.locks.RLock(ctx, key)
can fail and leave the outer deferred s.locks.RUnlock(key) unmatched; instead,
replace the deferred RLock in the hasReadLock branch with an immediate, checked
reacquire: after obtaining the write lock via s.locks.Lock(ctx, key) call
s.locks.RLock(ctx, key) and if that returns an error then s.locks.Unlock(key)
and return the error; if RLock succeeds immediately, then release the write lock
with s.locks.Unlock(key) (do not add another deferred RUnlock here since the
existing outer defer in GetWithConn will perform the single RUnlock) so the read
lock restore is validated before returning.
- Around line 443-450: The get() helper currently leaves reads unsynchronized
when called with noLock; change get() so that if lockState == noLock it acquires
the StorageImpl.RLock() for the entire read/decode phase (before gob decoding),
and only performs the upgrade to write lock (as already implemented) if
migration is required; this prevents saveObject() concurrent truncates/writes
from causing EOF/UnexpectedEOF during decoding and avoids passing corrupted
objects to the corruption cleanup. Update the logic in get() around the initial
read/gob decode to acquire and defer RUnlock when noLock is passed, and keep the
existing upgrade-to-write-lock path (and restoration to read lock) only when
migration triggers; ensure callers that pass noLock (e.g., the call sites in
containerprofile_storage.go and other storage.go callers) need no further
changes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d2cc08c1-1d21-487d-b548-3c142a512e8f

📥 Commits

Reviewing files that changed from the base of the PR and between 8bc5305 and 3f4b902.

📒 Files selected for processing (2)

• pkg/registry/file/containerprofile_storage.go
• pkg/registry/file/storage.go

[github-actions]

Summary:

• License scan: failure
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: failure

[github-actions]

Summary:

• License scan: failure
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: failure