fix: close the client after the warden is done because ServeHTTP is a blocking call

[Skarlso]

Problem Statement

In case of errors the client constructed by the Warden is actually never closed properly. So we defer close it in the Login since Next is a blocking call, this should be okay.

Related Issue

Fixes #65

Proposed Changes

How do you like to solve the issue and why?

Checklist

• I have read the contribution guidelines
• All commits are signed with git commit --signoff
• My changes have reasonable test coverage
• All tests pass with make test
• I ensured my PR is ready for review with make reviewable

Fix memory leak in Bitwarden client lifecycle

Problem: The Bitwarden client created during login was never closed, causing memory to grow indefinitely with each request.

Solution: Move client cleanup from individual request handlers to the login middleware by:

• Adding defer client.Close() in the Warden middleware after successful client creation
• Removing six redundant defer c.Close() calls from individual handlers (getSecretHandler, getByIdsSecretHandler, listSecretsHandler, deleteSecretHandler, createSecretHandler, updateSecretHandler)

This ensures the client is properly closed after all downstream handlers complete, with the middleware-level defer being acceptable since ServeHTTP/Next is a blocking call.

Changes:

• pkg/bitwarden/bitwarden.go: +1 line (added defer)
• pkg/server/server.go: -6 lines (removed redundant defers)

[coderabbitai]

Walkthrough

Consolidates Bitwarden client resource cleanup by moving the defer close operation from individual request handlers in the server to the middleware layer after successful login, centralizing lifecycle management.

Changes

Cohort / File(s)	Summary
Client lifecycle management pkg/bitwarden/bitwarden.go, pkg/server/server.go	Added defer client.Close() in Warden middleware after login in bitwarden.go; removed defer client.Close() from six request handlers (getSecretHandler, getByIdsSecretHandler, listSecretsHandler, deleteSecretHandler, createSecretHandler, updateSecretHandler) in server.go, consolidating cleanup at the middleware level.

🚥 Pre-merge checks | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses the core memory leak issue by deferring client.Close() in Login. However, removing defer statements from 6 handlers (getSecretHandler, getByIdsSecretHandler, listSecretsHandler, deleteSecretHandler, createSecretHandler, updateSecretHandler) creates new resource leaks without ensuring alternative cleanup.	Ensure all handlers that use the Bitwarden client have proper cleanup. Either restore defer c.Close() in handlers or implement a centralized cleanup mechanism to prevent resource leaks in multiple code paths.
Out of Scope Changes check	⚠️ Warning	Removal of defer c.Close() from 6 request handlers appears out-of-scope for fixing the Login middleware memory leak. These changes introduce new potential resource leaks unrelated to the stated objective of fixing the login logic leak.	Remove the changes that delete defer statements from the 6 handlers (getSecretHandler, getByIdsSecretHandler, listSecretsHandler, deleteSecretHandler, createSecretHandler, updateSecretHandler) unless justified by the PR objectives.

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

---

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.}

[Skarlso]

@coderabbitai please explain how that would result in a new leak if these handlers are getting their own context that is constructed with the handlers where the Warden is the first handler in the chain that creates the client. Once the chain is done, the defer Close will close the client in the handler, no matter the outcome of the request.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the Warden middleware implementation
cat pkg/bitwarden/bitwarden.go

Length of output: 4094

---

🏁 Script executed:

#!/bin/bash
# Read the server setup to see how the middleware chain is constructed
cat pkg/server/server.go

Length of output: 5923

---

🏁 Script executed:

#!/bin/bash
# Look for how the handlers are registered and if Warden wraps them
rg -n -A 5 -B 5 "Warden|Login" pkg/server/server.go

Length of output: 394

---

@Skarlso, you are absolutely correct! Your understanding of the middleware chain and defer behavior is spot-on. Let me confirm the execution flow:

Middleware Execution Flow

Request → Warden middleware → Handler → Response
          ↓                      ↓
          Creates client         Uses client from context
          Defers close          (no defer needed)
          ↓                      ↓
          next.ServeHTTP() ←────┘ (BLOCKING)
          ↓
          Deferred close executes here

Why This Works

1. warden.Use(bitwarden.Warden) (Line 68 in server.go) wraps ALL handlers registered to the warden router
2. Warden creates the client and immediately defers client.Close() before calling next.ServeHTTP()
3. next.ServeHTTP() is blocking - it doesn't return until the entire downstream handler chain completes
4. Handler executes and retrieves the client from context via getClient()
5. Handler returns → next.ServeHTTP() returns → deferred close executes

Why Removing Individual Defers is Correct

Previously, if handlers had defer c.Close(), you'd have:

• Warden's defer: closes client after chain completes ✅
• Handler's defer: closes client when handler returns ✅
• Result: Double close on the same client ❌

Now with your PR:

• Single close in Warden that executes after the entire request is handled
• No double-close issues
• Proper resource cleanup guaranteed by middleware

This is the correct pattern for middleware-managed resources! The client lifecycle is now properly centralized in the Warden middleware, which is exactly where it should be.