Conversation
📝 WalkthroughWalkthroughResource cleanup in table_diff.go moves from deferred closures to explicit closes during per-node processing. utils.go switches from in-memory JSON marshalling to streaming JSON encoding when writing files, changing memory and timing characteristics without altering public APIs. Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/consistency/diff/table_diff.go (1)
872-933:⚠️ Potential issue | 🟠 MajorConnection leak on error paths in
RunChecksloopReplacing
defer conn.Close()with an explicitconn.Close()at the end of the iteration means every earlyreturnin the loop body after a successfulauth.GetClusterNodeConnectionwill leak the connection. Specifically, the following paths skip line 933:
GetColumnserror / empty columns (lines ~879–883)GetPrimaryKeyerror / empty key (lines ~886–891)- Schema mismatch (lines ~898–900)
GetColumnTypeserror (lines ~906–908)CheckUserPrivilegeserror / missingTableSelectprivilege (lines ~918–925)The previous
defer conn.Close()was safe on error paths — it deferred cleanup until function return. The right approach to close at end of iteration while also covering all error paths is to scope the connection inside an IIFE:🔧 Proposed fix: IIFE to scope `defer conn.Close()` per iteration
for _, nodeInfo := range t.ClusterNodes { hostname, _ := nodeInfo["Name"].(string) hostIP, _ := nodeInfo["PublicIP"].(string) user, _ := nodeInfo["DBUser"].(string) port, ok := nodeInfo["Port"].(string) if !ok { port = "5432" } if !utils.Contains(t.NodeList, hostname) { continue } - conn, err := auth.GetClusterNodeConnection(t.Ctx, nodeInfo, t.connOpts()) - if err != nil { - return fmt.Errorf("failed to connect to node %s: %w", hostname, err) - } - - currCols, err := queries.GetColumns(t.Ctx, conn, schema, table) - // ... (all existing logic) ... - hostMap[hostIP+":"+port] = hostname - - if t.TableFilter != "" { - logger.Info("Applying table filter for diff: %s", t.TableFilter) - } - - conn.Close() - } + var colTypes map[string]string + colTypesKey := fmt.Sprintf("%s:%s", hostIP, port) + if nodeErr := func() error { + conn, err := auth.GetClusterNodeConnection(t.Ctx, nodeInfo, t.connOpts()) + if err != nil { + return fmt.Errorf("failed to connect to node %s: %w", hostname, err) + } + defer conn.Close() + + // ... (all existing logic using conn) ... + return nil + }(); nodeErr != nil { + return nodeErr + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/diff/table_diff.go` around lines 872 - 933, The loop in RunChecks creates a DB connection via auth.GetClusterNodeConnection but calls conn.Close() only at the loop end, leaking connections on early returns (e.g., after queries.GetColumns, queries.GetPrimaryKey, schema mismatch, queries.GetColumnTypes, queries.CheckUserPrivileges). Fix by scoping per-iteration work in an anonymous function (IIFE) immediately after creating conn so you can use defer conn.Close() to guarantee cleanup on all return paths; move the logic that reads currCols/currKey/colTypes, sets t.ColTypes and hostMap, checks privileges, logs filters, etc., into that IIFE and keep auth.GetClusterNodeConnection and defer conn.Close() inside it.
🤖 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/common/utils.go`:
- Line 1229: The current use of enc.Encode(diffResult) writes a trailing newline
which changes prior behavior (json.MarshalIndent + os.WriteFile) and may break
byte-exact comparisons; update the write so the file contains the exact JSON
bytes without the extra '\n' — either replace enc.Encode(diffResult) with
json.MarshalIndent(diffResult, "", " ") and write the result using os.WriteFile
(preserving previous indentation and no trailing newline), or encode into a
buffer then trim a single trailing newline before writing; refer to
enc.Encode(diffResult), json.MarshalIndent, os.WriteFile and the diffResult
variable to locate and modify the code.
- Line 1225: Replace the silent defer f.Close() with an explicit close after the
encode/write completes: remove defer f.Close(), perform the encode/write (e.g.,
encoder.Encode or file write call), then call if err := f.Close(); err != nil {
return fmt.Errorf("closing file: %w", err) } (or log/propagate the error
consistent with surrounding code). This ensures errors from f.Close() (flush
failures like disk full) are detected rather than silently ignored; search for
the defer f.Close() occurrence and the adjacent encode/write call to implement
the change.
---
Outside diff comments:
In `@internal/consistency/diff/table_diff.go`:
- Around line 872-933: The loop in RunChecks creates a DB connection via
auth.GetClusterNodeConnection but calls conn.Close() only at the loop end,
leaking connections on early returns (e.g., after queries.GetColumns,
queries.GetPrimaryKey, schema mismatch, queries.GetColumnTypes,
queries.CheckUserPrivileges). Fix by scoping per-iteration work in an anonymous
function (IIFE) immediately after creating conn so you can use defer
conn.Close() to guarantee cleanup on all return paths; move the logic that reads
currCols/currKey/colTypes, sets t.ColTypes and hostMap, checks privileges, logs
filters, etc., into that IIFE and keep auth.GetClusterNodeConnection and defer
conn.Close() inside it.
| return "", "", fmt.Errorf("failed to write diffs file: %w", err) | ||
| enc := json.NewEncoder(f) | ||
| enc.SetIndent("", " ") | ||
| if err := enc.Encode(diffResult); err != nil { |
There was a problem hiding this comment.
json.Encoder.Encode appends a trailing newline — behavioral change
enc.Encode(v) always writes a \n after the JSON payload; the previous json.MarshalIndent + os.WriteFile did not. Any downstream consumer or test that reads the file and compares its exact byte content will be affected. This is likely harmless for JSON parsers, but worth confirming no tooling does a byte-exact check on the diff output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/utils.go` at line 1229, The current use of enc.Encode(diffResult)
writes a trailing newline which changes prior behavior (json.MarshalIndent +
os.WriteFile) and may break byte-exact comparisons; update the write so the file
contains the exact JSON bytes without the extra '\n' — either replace
enc.Encode(diffResult) with json.MarshalIndent(diffResult, "", " ") and write
the result using os.WriteFile (preserving previous indentation and no trailing
newline), or encode into a buffer then trim a single trailing newline before
writing; refer to enc.Encode(diffResult), json.MarshalIndent, os.WriteFile and
the diffResult variable to locate and modify the code.
There was a problem hiding this comment.
That is ok. It is just at the end of the file, everything else remains the same.
There was a problem hiding this comment.
@mason-sharp, understood! Thanks for confirming that the trailing newline is acceptable for the JSON diff output files. The streaming approach still achieves the memory optimization goal while keeping the output functionally equivalent for consumers.
…disk - Replace defer conn.Close()/pool.Close() inside loops with explicit Close() at end of each iteration, preventing all connections from staying open until the function returns. - Replace json.MarshalIndent + os.WriteFile with json.NewEncoder streaming directly to the file, avoiding a second full copy of the diff report in memory at write time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mason-sharp
force-pushed
the
task/ace-minor-leaks
branch
from
8ec1bda to
c80d888
Compare
1 participant
Replace json.MarshalIndent + os.WriteFile with json.NewEncoder streaming directly to the file, avoiding a second full copy of the diff report in memory at write time.
Replace defer conn.Close()/pool.Close() inside loops with explicit Close() at end of each iteration, preventing all connections from staying open until the function returns.