feat(runtime): make fake runtime persistent (#65)#72
Merged
feloy merged 2 commits intokortex-hub:mainfrom Mar 16, 2026
Merged
Conversation
Implements the StorageAware interface for the fake runtime to persist instances to disk, allowing them to be discovered across multiple CLI runs. Instances are saved to instances.json and automatically loaded during initialization. The runtime remains backward compatible, operating in memory-only mode when not initialized with storage. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds disk-backed persistence to the fake runtime: new Initialize(storageDir) setup, JSON-serialized on-disk state, load/save logic, and updates to Create/Start/Stop/Remove/Info to persist instance lifecycle across process restarts. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/Client
participant RT as Fake Runtime
participant FS as File System
Note over CLI,RT: Initial run - create and start instance
CLI->>RT: Initialize(storageDir)
RT->>FS: loadFromDisk()
FS-->>RT: existing or empty persistedData
CLI->>RT: Create(config)
RT->>RT: allocate ID, create instanceState
RT->>FS: saveToDisk() (persist instance)
FS-->>RT: ack
CLI->>RT: Start(instanceID)
RT->>RT: update State/Info
RT->>FS: saveToDisk() (persist state)
FS-->>RT: ack
Note over CLI,RT: After restart - recover instances
CLI->>RT: Initialize(storageDir)
RT->>FS: loadFromDisk()
FS-->>RT: persistedData with instances
RT->>RT: restore in-memory instanceState(s)
CLI->>RT: Info(instanceID)
RT-->>CLI: return restored instance (ID, State, Info, Source, Config)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/runtime/fake/fake.go`:
- Around line 48-60: The persisted JSON currently uses flat fields (e.g.,
persistedData.NextID) which violates the nested-object schema rule; change
persistedData to embed a nested Meta (or Metadata) struct holding NextID (e.g.,
Meta struct { NextID int `json:"next_id"` }) and keep Instances as a
map[string]*instanceState so the top-level JSON becomes { "meta": { "next_id":
... }, "instances": { ... } }; update any code that marshals/unmarshals
persistedData and any accesses of persistedData.NextID or the file load/save
routines to use persistedData.Meta.NextID and ensure instanceState fields remain
as nested objects with appropriate json tags.
- Around line 79-89: In fakeRuntime.Initialize, normalize the incoming
storageDir to an absolute path before storing it: call filepath.Abs(storageDir),
return any error from Abs if it fails, then set f.storageDir to that absolute
path and compute f.storageFile with filepath.Join(absPath, storageFileName)
(preserve the existing locking around the assignment). This ensures
fakeRuntime.Initialize (function Initialize on type fakeRuntime and symbol
storageFileName) always stores an absolute path and uses filepath.Join for the
storage file.
- Around line 122-126: When restoring persisted state in fake.go (setting
f.nextID and f.instances), harden each loaded instanceState by ensuring its Info
map is non-nil so Start/Stop won't panic on inst.Info[...] assignments: after
assigning f.instances = persisted.Instances iterate over the map and for each
instance (type instanceState) if inst.Info == nil set inst.Info =
make(map[string]string) (or the correct value type), also apply the same non-nil
initialization for any other maps used by instance lifecycle methods referenced
by Start/Stop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85b8ff50-59b4-4018-9852-b9090039af02
📒 Files selected for processing (2)
pkg/runtime/fake/fake.gopkg/runtime/fake/fake_test.go
After loading instances from disk, ensure the Info map is non-nil to prevent panics in Start/Stop methods. Adds defensive initialization and comprehensive test coverage for corrupted storage scenarios. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/runtime/fake/fake.go (1)
152-159: Consider atomic write to prevent file corruption.
os.WriteFiledirectly overwrites the file, which can leave it corrupted if the process crashes mid-write. For a test runtime this may be acceptable, but atomic write (write to temp file, then rename) is safer.♻️ Optional: Atomic write pattern
- if err := os.WriteFile(f.storageFile, data, 0644); err != nil { - return fmt.Errorf("failed to write storage file: %w", err) - } + tmpFile := f.storageFile + ".tmp" + if err := os.WriteFile(tmpFile, data, 0644); err != nil { + return fmt.Errorf("failed to write storage file: %w", err) + } + if err := os.Rename(tmpFile, f.storageFile); err != nil { + return fmt.Errorf("failed to finalize storage file: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/fake/fake.go` around lines 152 - 159, The current write uses os.WriteFile which can corrupt f.storageFile if interrupted; change the persistence logic (the block that marshals persisted with json.MarshalIndent and writes to f.storageFile) to perform an atomic write: write the marshaled data to a uniquely named temp file in the same directory (e.g., f.storageFile+".tmp" or use os.CreateTemp), fsync the temp file (File.Sync) and close it, then atomically replace the target by os.Rename(tempPath, f.storageFile); also handle cleanup on errors and propagate failures (preserve the existing error wrapping messages).
🤖 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/runtime/fake/fake.go`:
- Around line 209-214: The code assigns f.instances[id] and then calls
saveToDisk(), which can fail and leave in-memory state inconsistent; modify the
routine so that if saveToDisk() returns an error you roll back the in-memory
mutation: delete f.instances[id] and decrement f.nextID (or restore its previous
value) before returning the fmt.Errorf; do this where f.instances[id] = state
and the subsequent saveToDisk() call occur, referencing f.instances, f.nextID
and saveToDisk() to locate the change and ensure the rollback happens under the
same lock/guard used for mutations.
- Around line 122-137: When loading persisted state into f.nextID and
f.instances, defend against nil map values and invalid nextID: iterate
f.instances and for any nil entry (value == nil) allocate a new instanceState
(with Info initialized) and assign it back into f.instances to avoid panics when
accessing inst.Info; also ensure inst.Info is non-nil for existing entries;
finally validate persisted.NextID and if it's <= 0 set f.nextID to 1 (or max(1,
persisted.NextID)) to avoid ID collisions. Apply these changes around where
f.nextID = persisted.NextID and the loop over f.instances and use the
instanceState and Info symbols to locate the code to update.
---
Nitpick comments:
In `@pkg/runtime/fake/fake.go`:
- Around line 152-159: The current write uses os.WriteFile which can corrupt
f.storageFile if interrupted; change the persistence logic (the block that
marshals persisted with json.MarshalIndent and writes to f.storageFile) to
perform an atomic write: write the marshaled data to a uniquely named temp file
in the same directory (e.g., f.storageFile+".tmp" or use os.CreateTemp), fsync
the temp file (File.Sync) and close it, then atomically replace the target by
os.Rename(tempPath, f.storageFile); also handle cleanup on errors and propagate
failures (preserve the existing error wrapping messages).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b306ff8-dfa9-4426-84da-1be0cc21336e
📒 Files selected for processing (2)
pkg/runtime/fake/fake.gopkg/runtime/fake/fake_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/runtime/fake/fake_test.go
benoitf
approved these changes
Mar 16, 2026
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
3 participants
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.
Implements the StorageAware interface for the fake runtime to persist instances to disk, allowing them to be discovered across multiple CLI runs. Instances are saved to instances.json and automatically loaded during initialization. The runtime remains backward compatible, operating in memory-only mode when not initialized with storage.
fixes #65