🔥feat: Add storage backed SharedState for prefork applications#4243
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a storage-backed, prefork-safe SharedState to App with Config options ( Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant SS as SharedState
participant St as Storage
rect rgba(200,230,255,0.5)
App->>SS: SharedState().SetJSON(key, value, ttl)
SS->>SS: encode JSON (configured encoder)
SS->>St: Set(prefixedKey, bytes, ttl)
St-->>SS: ok / error
SS-->>App: return error or nil
end
rect rgba(230,255,200,0.5)
App->>SS: SharedState().GetJSON(key)
SS->>St: Get(prefixedKey)
St-->>SS: bytes / not-found / error
SS->>SS: decode JSON (configured decoder)
SS-->>App: decoded value, found flag, error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4243 +/- ##
==========================================
+ Coverage 91.23% 91.25% +0.02%
==========================================
Files 126 127 +1
Lines 12341 12553 +212
==========================================
+ Hits 11259 11455 +196
- Misses 678 687 +9
- Partials 404 411 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SharedState, a storage-backed state management system for Fiber designed to be safe for prefork workers and multi-process environments. It adds new configuration fields to Config, implements a variety of data access methods with support for multiple encoding formats (JSON, MsgPack, CBOR, XML), and provides comprehensive documentation and tests. Review feedback suggests removing a redundant string copy in the key generation logic and highlights potential concerns regarding key length and readability when using hex encoding.
There was a problem hiding this comment.
Pull request overview
Adds a new storage-backed SharedState API to Fiber apps to support prefork-safe / multi-process shared key–value state, with configurable namespacing and codec helpers aligned with app-configured encoders/decoders.
Changes:
- Introduces
SharedStatewith raw (Set/Get) and encoded (SetJSON/GetJSON,SetMsgPack/GetMsgPack,SetCBOR/GetCBOR,SetXML/GetXML) operations, plus context-aware variants. - Wires shared state into
AppviaConfig.SharedStorageandConfig.SharedStatePrefix, and exposesApp.SharedState()alongside clarifiedApp.State()semantics. - Adds documentation and a comprehensive unit test suite covering configuration, namespacing, context propagation, and error handling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
shared_state.go |
Implements the SharedState API over fiber.Storage, including prefixing and codec helpers. |
app.go |
Adds shared-state configuration, initializes shared state in New(), and exposes App.SharedState(). |
shared_state_test.go |
Adds unit tests for shared-state behavior (storage config, namespacing, codecs, context, error propagation). |
docs/api/state.md |
Documents prefork-safe SharedState, configuration, method signatures, and examples. |
docs/api/app.md |
Updates app API docs to distinguish State() (process-local) vs SharedState() (storage-backed). |
docs/whats_new.md |
Notes SharedState as a new v3 app feature. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 7b016c0 | Previous: fbc03bd | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd (github.com/gofiber/fiber/v3/middleware/compress) - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Good feature idea, storage-backed shared state for prefork is a real need. The integration with the existing fiber.Storage interface and app config is clean. A few things to address before merge:
1. Nil encoder panic (Bug - Medium)
Only JSON gets defaults in newSharedState:
if jsonEncoder == nil { jsonEncoder = json.Marshal }
if jsonDecoder == nil { jsonDecoder = json.Unmarshal }MsgPack, CBOR, and XML encoders have no nil guard. Calling SetMsgPack("key", val, ttl) without configuring Config.MsgPackEncoder will panic with a nil pointer dereference.
Should either return a descriptive error (e.g. ErrMsgPackEncoderNotConfigured) or add nil checks before calling the encoder/decoder in each method.
2. Massive code duplication (Design - Major)
SetJSON/GetJSON, SetMsgPack/GetMsgPack, SetCBOR/GetCBOR, SetXML/GetXML are near-identical — ~200 lines of copy-paste where only the encoder/decoder function and error string differ. This could be reduced to two internal helpers:
func (s *SharedState) setEncoded(ctx context.Context, key string, v any, ttl time.Duration,
encoder func(any) ([]byte, error), format string) error {
if s == nil || s.storage == nil {
return ErrSharedStorageNotConfigured
}
if key == "" {
return nil
}
encoded, err := encoder(v)
if err != nil {
return fmt.Errorf("fiber: failed to encode shared state %s value: %w", format, err)
}
return s.storage.SetWithContext(ctx, s.key(key), encoded, ttl)
}
func (s *SharedState) getEncoded(ctx context.Context, key string, out any,
decoder func([]byte, any) error, format string) ([]byte, bool, error) {
if s == nil || s.storage == nil {
return nil, false, ErrSharedStorageNotConfigured
}
data, err := s.storage.GetWithContext(ctx, s.key(key))
if err != nil {
return nil, false, err
}
if data == nil {
return nil, false, nil
}
if err := decoder(data, out); err != nil {
return nil, false, fmt.Errorf("fiber: failed to decode shared state %s value: %w", format, err)
}
return append([]byte(nil), data...), true, nil
}Each public method becomes a one-liner:
func (s *SharedState) SetJSONWithContext(ctx context.Context, key string, v any, ttl time.Duration) error {
return s.setEncoded(ctx, key, v, ttl, s.jsonEncoder, "json")
}This also makes the nil-encoder check (#1) fixable in a single place.
3. Inconsistent empty-key handling (Bug - Medium)
Set("")→ calls storage with empty key (key("")returns"", bypassing prefix)SetJSON("")→ silently returnsnil(guarded)Get("")→ calls storage with empty keyDelete("")→ calls storage with empty keyHas("")→ calls storage with empty key
The encoded variants guard against empty keys, but raw Set/Get/Delete/Has do not. This should be consistent — either guard in key() itself or in all public methods.
Copilot flagged this too — the key() function returning "" for empty input effectively bypasses the prefix, which is likely unintended.
4. newSharedState takes 10 parameters (Style - Low)
Could take a subset of Config or a small options struct instead:
func newSharedState(storage Storage, prefix string, cfg Config) *SharedState5. Dead code: double prefix defaulting (Low)
app.go already ensures a non-empty prefix before calling newSharedState:
sharedStatePrefix := app.config.SharedStatePrefix
if sharedStatePrefix == "" {
sharedStatePrefix = defaultSharedStatePrefix
// ...
}The fallback in newSharedState (if prefix == "" { prefix = defaultSharedStatePrefix }) is unreachable when called from New().
6. No Reset()/Close() forwarding (Low)
The Storage interface has Reset() and Close(). SharedState doesn't expose these. If a user needs to clean up shared state or close the storage connection via SharedState, they currently can't. Consider at least a Close() method, or document that the caller manages the storage lifecycle directly.
ReneWerner87
left a comment
ReneWerner87
left a comment
There was a problem hiding this comment.
Inline review comments on specific locations.
|
@copilot address the comments made on this post #4243 (review) |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/ae846c95-4e83-4e0a-87ee-1b7b9c3deaa3 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app.go`:
- Around line 93-94: The SharedState instance (sharedState) is never closed
during shutdown; call sharedState.Close() when the app is shutting down — either
add a call to sharedState.Close() in ShutdownWithContext or invoke it in the
existing OnPostShutdown hook alongside services shutdown so storage backend
resources are released; reference the sharedState variable and the
SharedState.Close() method and ensure you handle/propagate any returned error
(log or return) consistent with existing shutdown error handling.
In `@shared_state_test.go`:
- Around line 697-734: The test TestSharedState_EmptyKeyBehavior calls
SetMsgPack and SetCBOR but creates the app without MsgPackEncoder/CBOREncoder,
so those calls should return errors per
TestSharedState_UnconfiguredCodecsReturnErrorInsteadOfPanic; fix by either
removing the SetMsgPack/SetCBOR (and corresponding GetMsgPack/GetCBOR)
assertions from this test, or instantiate the app with MsgPackEncoder and
CBOREncoder set in New(Config{...}) so SetMsgPack and SetCBOR succeed—update the
test to use one of these two approaches and adjust expected error/no-error
assertions accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be2632c8-a481-4929-a958-2dc99cdc8ff8
📒 Files selected for processing (5)
app.godocs/api/state.mddocs/whats_new.mdshared_state.goshared_state_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/whats_new.md
- docs/api/state.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shared_state.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/ae846c95-4e83-4e0a-87ee-1b7b9c3deaa3 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
... Addressed in a4946e9: the encoded helpers now share common set/get paths, empty keys are handled consistently across raw and encoded methods,
Addressed in a4946e9. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app.go (1)
1384-1390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose
SharedStateduring shutdown.
OnPostShutdownstill only tears down services. The new storage-backedsharedStatecan keep backend resources open unless it is closed here too.🔧 Suggested fix
app.Hooks().OnPostShutdown(func(_ error) error { + if app.sharedState != nil && app.config.SharedStorage != nil { + if err := app.sharedState.Close(); err != nil { + log.Errorf("failed to close shared state: %v", err) + } + } if err := app.shutdownServices(app.servicesShutdownCtx()); err != nil { log.Errorf("failed to shutdown services: %v", err) } return nil })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.go` around lines 1384 - 1390, OnPostShutdown currently only calls app.shutdownServices; also close the storage-backed shared state there. In the app.Hooks().OnPostShutdown callback, after calling app.shutdownServices(app.servicesShutdownCtx()), check if app.sharedState (or the field that holds the SharedState) is non-nil and call its Close() method, logging any error (e.g., log.Errorf("failed to close sharedState: %v", err)). Ensure you perform the Close() call regardless of service shutdown result and return nil as before.shared_state_test.go (1)
703-730:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop the MsgPack/CBOR empty-key assertions or configure the codecs.
New(Config{SharedStorage: ...})leaves MsgPack/CBOR unset here, so theseSetMsgPack/SetCBORcalls will return the shared-state codec error instead of succeeding.🔧 Suggested fix
require.NoError(t, app.SharedState().Set("", []byte("raw"), time.Minute)) require.NoError(t, app.SharedState().SetJSON("", Map{"v": 1}, time.Minute)) - require.NoError(t, app.SharedState().SetMsgPack("", Map{"v": 1}, time.Minute)) - require.NoError(t, app.SharedState().SetCBOR("", Map{"v": 1}, time.Minute)) require.NoError(t, app.SharedState().SetXML("", Map{"v": 1}, time.Minute)) @@ _, found, err = app.SharedState().GetJSON("", &Map{}) require.NoError(t, err) require.False(t, found) - - _, found, err = app.SharedState().GetMsgPack("", &Map{}) - require.NoError(t, err) - require.False(t, found) - - _, found, err = app.SharedState().GetCBOR("", &Map{}) - require.NoError(t, err) - require.False(t, found) _, found, err = app.SharedState().GetXML("", &Map{}) require.NoError(t, err) require.False(t, found)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared_state_test.go` around lines 703 - 730, The failing assertions come from calling SetMsgPack/SetCBOR and GetMsgPack/GetCBOR while the App was created with New(Config{SharedStorage: newSharedStateMemoryStorage(t)}) without MsgPack/CBOR codecs; either remove the MsgPack/CBOR Set*/Get* assertions from the test or provide codecs in the Config passed to New (e.g., set the MsgPackCodec and CBORCodec fields) so that SetMsgPack, SetCBOR, GetMsgPack and GetCBOR succeed; locate these calls and update the test to configure codecs on Config or drop the codec-specific assertions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app.go`:
- Around line 1384-1390: OnPostShutdown currently only calls
app.shutdownServices; also close the storage-backed shared state there. In the
app.Hooks().OnPostShutdown callback, after calling
app.shutdownServices(app.servicesShutdownCtx()), check if app.sharedState (or
the field that holds the SharedState) is non-nil and call its Close() method,
logging any error (e.g., log.Errorf("failed to close sharedState: %v", err)).
Ensure you perform the Close() call regardless of service shutdown result and
return nil as before.
In `@shared_state_test.go`:
- Around line 703-730: The failing assertions come from calling
SetMsgPack/SetCBOR and GetMsgPack/GetCBOR while the App was created with
New(Config{SharedStorage: newSharedStateMemoryStorage(t)}) without MsgPack/CBOR
codecs; either remove the MsgPack/CBOR Set*/Get* assertions from the test or
provide codecs in the Config passed to New (e.g., set the MsgPackCodec and
CBORCodec fields) so that SetMsgPack, SetCBOR, GetMsgPack and GetCBOR succeed;
locate these calls and update the test to configure codecs on Config or drop the
codec-specific assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9c645499-2e14-436c-9432-534bbad2540e
📒 Files selected for processing (3)
app.goshared_state.goshared_state_test.go
|
@copilot Verify each finding against the current code and only fix it if needed. In |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7d092650-1040-458d-9f36-30712bdc39a0 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7d092650-1040-458d-9f36-30712bdc39a0 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Verified the finding against the current code and fixed it in 34c4bc9: the app post-shutdown hook now closes configured shared state after service shutdown, logs any close error, and still attempts the close even when service shutdown reports an error. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services_test.go`:
- Around line 260-308: The tests set log.SetOutput to a dummy buffer but restore
it incorrectly; capture the current log output before calling log.SetOutput and
restore that captured writer in t.Cleanup (replace the bytes.NewBuffer(nil)
restore) so later tests aren’t affected, and add t.Parallel() at the start of
each subtest (both "shutdown-hooks/close-shared-state" and
"shutdown-hooks/close-shared-state-after-service-error") to run them in parallel
once the logger sink is isolated; relevant symbols to modify: the two subtest
bodies, the log.SetOutput call, the t.Cleanup restore, and ensure this change is
applied alongside the existing use of shutdownHookStorage, app.initServices, and
app.Hooks().executeOnPostShutdownHooks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 737916fa-4177-4a52-a674-af20c29d73d0
📒 Files selected for processing (2)
app.goservices_test.go
|
@copilot Verify each finding against the current code and only fix it if needed. In |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/e5263c7d-b450-40dd-9aaf-31d81e0efb5d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/e5263c7d-b450-40dd-9aaf-31d81e0efb5d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in 6543581: the two shutdown-hook subtests now call t.Parallel(), capture and restore the previous logger writer instead of restoring to a dummy buffer, and use a panic-safe helper plus mutex so the temporary global log sink stays isolated. |
|
@claude[agent] Audit all the changes made in this Pull Request to implement the SharedState feature. |
SharedState Implementation Audit ReportI've completed a comprehensive audit of all changes in this PR implementing the SharedState feature. All implementation aspects are correct and complete. ✅ Core Implementation (shared_state.go - 416 lines)Architecture:
Methods Implemented:
Code Quality Improvements (from review feedback):
✅ App Integration (app.go - 31 lines changed)Configuration:
Shutdown Hook:
✅ Test Coverage (shared_state_test.go - 737 lines)20 test functions covering:
Shutdown hook tests (services_test.go - 127 lines added):
✅ Documentation (141 lines added)docs/api/state.md:
docs/api/app.md:
docs/whats_new.md:
✅ All Quality Checks Pass
Key Design Decisions
The implementation is production-ready and fully addresses the review feedback from @ReneWerner87 and @copilot-pull-request-reviewer. |
ReneWerner87
deleted the
implement-shared-state-component-using-fiber.storage
branch
Motivation
AppName) and reuse app-configured encoders/decoders for JSON/MsgPack/CBOR/XML.Description
SharedStateimplementation (shared_state.go) exposing raw and encoded helpers such asSet,Get,SetJSON,GetJSON,SetMsgPack,GetMsgPack,SetCBOR,GetCBOR,SetXML,GetXML,Delete,Has, and context-aware variants.SharedStateinto the app by addingConfig.SharedStorageandConfig.SharedStatePrefix, initializingapp.sharedStateinNew(...), and exposingApp.SharedState()while clarifying thatApp.State()remains process-local.defaultSharedStatePrefix+ optionalAppName) and ensure theSharedStateuses the app-provided encoder/decoder hooks.docs/api/app.md,docs/api/state.md,docs/whats_new.md) and add comprehensive unit tests inshared_state_test.gocovering configuration, context propagation, serialization, codec usage, namespacing, and error propagation.