Repository navigation
Clean up staged outputs when mmdbconvert is interrupted (STF-1809) - #116
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe conversion API and merger now accept contexts. Cancellation checks cover conversion, output preparation, writes, and publication. The CLI handles termination signals with platform-specific behavior. CSV and Parquet writers and format detection now handle ChangesConversion cancellation
Uint128 writer conversion
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CLI as runCLI
participant Signals as runWithSignalChannel
participant Run as mmdbconvert.Run
participant Merger as Merger.Merge
participant Output as flushAndCommit
CLI->>Signals: Start conversion callback
Signals->>Run: Provide cancel-cause context
Run->>Merger: Pass context to Merge
Signals->>Run: Cancel context on first handled signal
Run->>Output: Pass context to publication
Signals->>Signals: Exit immediately on a second handled signal
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds context cancellation and signal-driven cleanup with tests and documentation. No actionable merge-blocking issue remains in the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Staged publication reduces the chance that an interrupted conversion replaces existing output with incomplete data. The review found no demonstrated new security exposure, but the public API and interruption behavior warrant design review, and some deployment-specific effects remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 112 functions across 32 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit taps the context bell, Comment |
cecdf38 to
0c98293
Compare
7106b73 to
2ed86bc
Compare
85c2be9 to
d822ddd
Compare
32a851e to
3788e22
Compare
9c83bf7 to
4c9c4f7
Compare
3788e22 to
1f63377
Compare
155a93f to
78cf7fb
Compare
1f63377 to
56b0721
Compare
78cf7fb to
a736943
Compare
56b0721 to
83488a2
Compare
42e38b7 to
90d94cf
Compare
a5476d6 to
3d3e38e
Compare
oschwald
changed the title
Require a context in Run and keep library calls free of signal handlers. Check cancellation through merging, output writes, and serialization; clean unpublished files and preserve cleanup errors. Finish publication once it starts. Update existing callers for the breaking API change. Cover all formats and split outputs, production writer capabilities, final accumulator flush cancellation, cleanup failures, and publication completion.
Cancel conversion on handled signals while preserving inherited ignored SIGINT and SIGHUP. Drain queued signals at shutdown, identify interruptions separately from conversion errors, and allow a second signal to terminate blocked cleanup. Re-raise Unix signals after cleanup and use the native Windows console exit status. Include platform signal tests and document cooperative cleanup, duplicate forwarding, and numeric fallbacks.
3a549f0 to
b7b11a8
Compare
horgh
left a comment
horgh
left a comment
There was a problem hiding this comment.
LGTM, but Claude comments.
| case *mmdbtype.Uint128: | ||
| return (*big.Int)(v).String(), nil | ||
| case mmdbtype.Uint128: | ||
| return v.BigInt().String(), nil |
There was a problem hiding this comment.
This fix covers only a top-level Uint128. A Uint128 inside a Map or Slice goes to json.Marshal in the mmdbtype.Map and mmdbtype.Slice cases below (line 434). After the mmdbwriter update, Uint128 is a value struct, so JSON encodes its internal fields.
For example, {"n": Uint128{Low: 42}} now gives {"n":{"High":0,"Low":42}}. Before the update it gave {"n":{}}. Neither is the decimal value 42. This also affects Parquet string columns that select a map. No test covers the nested case.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in e2fec23. Nested maps and slices now encode Uint128 values as exact, unquoted decimal JSON numbers without changing the input. This covers CSV and Parquet string columns, including the default string conversion. Tests cover nested and maximum values, shared containers, cycles, and nonfinite values; JSON encoding errors remain intact.
🤖 Comment by Codex on behalf of Greg.
| @@ -0,0 +1,37 @@ | |||
| //go:build !windows | |||
There was a problem hiding this comment.
!windows also selects js/wasm and plan9. These targets do not have syscall.SIGHUP, syscall.Kill or syscall.Signal, so the CLI no longer compiles for them:
$ GOOS=js GOARCH=wasm go build ./cmd/mmdbconvert
cmd/mmdbconvert/signals_unix.go:16:73: undefined: syscall.SIGHUP
$ GOOS=plan9 go build ./cmd/mmdbconvert
cmd/mmdbconvert/signals_unix.go:28:20: undefined: syscall.Kill
Both build on main. They are not in the release list, so the effect is small. //go:build unix, plus a fallback file for the other targets, fixes it.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Not sure about this, but I guess it is fine to add support for them.
There was a problem hiding this comment.
Fixed in 790e1d1. The Unix implementation and tests now use //go:build unix; other non-Windows platforms keep their native signal handling through a fallback that installs no handlers. CLI test compilation passes for Windows, macOS, Plan 9, js/wasm, and wasip1/wasm. This preserves existing builds without adding release targets.
🤖 Comment by Codex on behalf of Greg.
| } | ||
| code, cause := runWithSignalChannel(ctx, run, signals, logger) | ||
| if cause != nil { | ||
| logger.Warn("Conversion interrupted", "cause", cause) |
There was a problem hiding this comment.
This warning is logged whenever the context has a cause, even when the conversion finished and published its outputs. If SIGTERM arrives during Commit, or just after run returns, the log shows "Successfully completed" and then "Conversion interrupted", and the process exits by signal. An operator cannot tell from the log if the outputs were replaced.
Also, this call uses the key cause. Other log calls use the key error, so JSON log consumers that look for error miss this one.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 5a22ec6. A signal with a successful callback now logs Signal received; conversion completed and outputs were published. A nonzero result logs Signal received, since profiling or publication errors can occur after outputs were replaced. Both use error. Unrelated parent cancellation logs Conversion context canceled. Tests cover both signal outcomes and the JSON fields. Native signal termination is preserved even after successful publication so interrupted shell loops stop.
🤖 Comment by Codex on behalf of Greg.
| case *mmdbtype.Uint128: | ||
| i := (*big.Int)(v) | ||
| case mmdbtype.Uint128: | ||
| i := v.BigInt() |
There was a problem hiding this comment.
v.BigInt() allocates a big.Int for each Uint128 cell. The same is true in the float64 case at line 421 and in convertToString at csv.go:426. The old pointer conversion did not allocate, and the new value type makes the allocation unnecessary. For example:
case mmdbtype.Uint128:
if v.High != 0 || v.Low > math.MaxInt64 {
return nil, fmt.Errorf("uint128 value %s overflows int64", v.BigInt())
}
return int64(v.Low), nilIn the CSV path, strconv.FormatUint(v.Low, 10) works when v.High == 0.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Hmm, not sure we would want to error if the uint128 is not a int64.
There was a problem hiding this comment.
Fixed in 315438d. Low-word values use direct int64/float64 conversions and strconv.FormatUint, avoiding BigInt. High-word float64 conversion keeps BigInt.Float64 to avoid double rounding; a regression test covers that case.
The overflow error applies only when the column explicitly requests type = "int64", preserving the existing range check. Uint128 values outside int64 still work in CSV and Parquet string columns, including the default string conversion.
🤖 Comment by Codex on behalf of Greg.
| } | ||
| } | ||
|
|
||
| func TestCLISecondSignal(t *testing.T) { |
There was a problem hiding this comment.
TestCLISignals, TestCLISecondSignal and TestCLIInterruptStopsShellLoop each repeat the same subprocess setup: os.Executable, exec.CommandContext, the environment, the stderr buffer, StdoutPipe, Start, and the read of ready. That is about 15 lines, three times. A change to the handshake must be made in three places.
TestCLISignals is TestCLISecondSignal without the second signal, so those two can be one table-driven test. A shared helper can start the child for all three.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 2c32b0e. Single- and second-signal cases now share one table. A common subprocess helper also serves the shell-loop test and owns the timeout, readiness handshake, buffered reader, stderr capture after Wait, and cleanup/reaping. Repeated signal tests pass with the race detector. Checks with inherited ignored SIGINT/SIGHUP and missing Bash also pass.
🤖 Comment by Codex on behalf of Greg.
| }) | ||
| } | ||
|
|
||
| func runConversion( |
There was a problem hiding this comment.
Nit: runCLI calls extraConfigArgsMessage first (line 139) and runConversion later (line 165). The project CLAUDE.md orders helpers by first call, so runConversion goes after extraConfigArgsMessage.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 194661f: extraConfigArgsMessage now precedes runConversion, matching their first-call order.
🤖 Comment by Codex on behalf of Greg.
| _, cfg, paths := outputTestConfig(t, "mmdb", false, `["country_code"]`) | ||
| require.NoError(t, os.WriteFile(paths[0], []byte("previous output"), 0o600)) | ||
| // A single data range stays in the accumulator until Merge's final Flush. | ||
| tree, err := mmdbwriter.New(mmdbwriter.Options{IPVersion: 4}) |
There was a problem hiding this comment.
Nit: createCSVTestDatabase in mmdbconvert_test.go already builds a small MMDB for tests in this package. createCSVTestDatabase(t, 4, []string{"1.0.0.0/24"}) with a ["value"] column path gives the same single-range input with less code.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 14e276b using createCSVTestDatabase(t, 4, []string{"1.0.0.0/24"}) and the ["value"] path. The existing cancellation assertions remain. A source-overlay mutation removing the pre-serialization cancellation check still makes this test fail because Flush is called.
🤖 Comment by Codex on behalf of Greg.
The PR check found a newer upstream commit after the review fixes were pushed. Update the existing pseudo-version to cc514d224474; the API remains compatible.
horgh
left a comment
horgh
left a comment
There was a problem hiding this comment.
I ran another Claude review. Feel free to re-deliver if you like.
| } | ||
| if interrupted { | ||
| //revive:disable-next-line:deep-exit A second signal must bypass blocked cleanup. | ||
| os.Exit(exitAfterSignal(sig, logger)) |
There was a problem hiding this comment.
One user interrupt that arrives twice counts as the second signal. The CLI then exits at once and skips cleanup.
This can happen in two ways:
- A wrapper in the same process group forwards SIGINT (for example, npm or pnpm runners, or some init shims). Ctrl-C then delivers SIGINT from the terminal and again from the wrapper.
- A terminal closes. The kernel sends SIGHUP to the foreground group, and the shell also sends SIGHUP to its jobs.
The first copy cancels the context. The second copy either is queued before signal.Stop and reaches this os.Exit, or arrives after Stop and gets the default action. Both paths end the process before the deferred cleanup in convert. The .mmdbconvert-* staged files then stay next to the output, and split publication can stop partway through.
A short grace period, or ignoring a repeat of the same signal for a short time, would separate a duplicate delivery from a deliberate second signal.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Doesn't seem very important.
There was a problem hiding this comment.
Keeping immediate termination on a second handled signal, as agreed for this PR. A grace period would delay the escape from blocked cleanup. The configuration documentation already explains that direct delivery plus wrapper forwarding can trigger this with one Ctrl-C and leave staged files behind.
🤖 Comment by Codex on behalf of Greg.
| // so a second signal can terminate blocked cleanup. | ||
| signal.Stop(signals) | ||
| interrupted = true | ||
| cancel(&signalError{signal: sig}) |
There was a problem hiding this comment.
If the parent context is already canceled, cancel(&signalError{...}) does nothing. context.WithCancelCause keeps the parent's cause.
For example, the caller of runCLI(ctx) cancels ctx, and then SIGINT arrives. This goroutine reads the SIGINT, calls signal.Stop, and calls cancel. context.Cause returns the parent's error, so errors.AsType[*signalError] in runWithSignals fails. The CLI logs "Conversion context canceled" and returns 1. It does not re-raise SIGINT. A shell loop around the process sees a normal exit 1, and the interrupt is lost.
The goroutine could record the signal in a variable that runWithSignals checks, instead of using only the context cause.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Not sure this matters either.
There was a problem hiding this comment.
Fixed in 1331f04. The watcher records the first received signal independently of the context cause, and the caller reads it after the watcher finishes. The callback retains the original parent cause, while a received signal still controls process termination. Regression tests cover an already-canceled parent with callback results 0 and 1.
🤖 Comment by Codex on behalf of Greg.
| return exitAfterSignal(interruption.signal, logger) | ||
| } | ||
| if cause != nil { | ||
| logger.Warn("Conversion context canceled", "error", cause) |
There was a problem hiding this comment.
Follow-up to the earlier thread about the success log: this branch still logs a cancellation warning when the conversion succeeded.
If the parent context is canceled after mmdbconvert.Run returns nil, but before runWithSignalChannel reads context.Cause (for example, while the memory profile is written), the CLI returns 0. It also logs a WARN record that says the conversion context was canceled. The signal branch above checks code. This branch does not.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 1331f04: a non-signal parent cancellation warning is now emitted only for a nonzero callback result. Tests cover both successful and unsuccessful callbacks. Signal diagnostics and native signal termination still apply after successful publication.
🤖 Comment by Codex on behalf of Greg.
| require.Equal(t, "first", buf.String()) | ||
| } | ||
|
|
||
| func TestConvert_CancellationDuringWrite(t *testing.T) { |
There was a problem hiding this comment.
This test does not test the check in contextWriter.Write. I removed the ctx.Err() check from contextWriter.Write, and all three subtests still passed.
With this small fixture, the CSV and MMDB writers buffer all output and send it in one Write when Flush runs. cancelWriteOutput cancels only after that write succeeds. The error therefore comes from the context check after the flush in flushAndCommit. Parquet also passes. Only the flush stage of TestConvert_CancellationCleansOutputs catches a regression in contextWriter.
To test the write check, cancel after the first write and make sure a second write happens. For example, use a writer that cancels on its first Write, or a fixture that is larger than the buffer.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Renamed this to TestConvert_CancellationAfterOutputWrite and clarified its purpose in 9790cfe: cancellation after a successful underlying write must preserve the old output and remove staged files. The existing TestContextWriter_Cancellation already writes once, cancels, attempts a second write, and checks zero additional bytes. I confirmed with a source-overlay mutation that removing the adapter check makes that test fail. The real-serializer flush tests remain in place.
🤖 Comment by Codex on behalf of Greg.
| // Run does not install signal handlers. | ||
| func Run(ctx context.Context, opts Options) error { | ||
| if err := ctx.Err(); err != nil { | ||
| return fmt.Errorf("starting conversion: %w", err) |
There was a problem hiding this comment.
The cancellation errors wrap ctx.Err(), not context.Cause(ctx). A library caller does not get the cause back from Run.
For example, a caller uses context.WithCancelCause and cancels with errors.New("shutdown requested"). Run returns starting conversion: context canceled, and the caller's cause is not in the error chain. The same is true at output.go:227 ("writing output"), in the merger, and in flushAndCommit. The CLI reads context.Cause separately to work around this. Any other caller must keep the context and do the same.
context.Cause(ctx) wraps context.Canceled when no cause was set, so errors.Is(err, context.Canceled) still works if these sites use it.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Keeping the standard ctx.Err() contract and documenting it in e666831. Returned cancellation errors remain discoverable as context.Canceled or context.DeadlineExceeded; callers can inspect their custom reason through context.Cause(ctx). Replacing ctx.Err() with context.Cause() alone would lose that standard error matching when the custom cause does not wrap the sentinel.
🤖 Comment by Codex on behalf of Greg.
| t.Cleanup(func() { | ||
| if opts.shellLoop { | ||
| // Kill any surviving second child even if the shell was already reaped. | ||
| if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { |
There was a problem hiding this comment.
This cleanup can send SIGKILL to a process group ID that another process has reused.
In TestCLIInterruptStopsShellLoop, child.wait reaps bash, which is the group leader. If no member of the group is still alive, the kernel can give that PGID to a new group, for example a parallel test or a CI process. syscall.Kill(-pid, SIGKILL) then kills that unrelated group. This is unlikely, but it can happen on busy CI hosts.
It is safer to kill the group before the leader is reaped, or only while a group member is known to be alive.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in 9a64fc3. Removed both the cleanup group kill and the custom group-killing cancellation callback. Shell-loop helpers inherit a stdin pipe and exit when it closes; an independent context callback closes it on timeout even after Bash is reaped. Cleanup also closes it and reaps the command once. A regression test kills only Bash, then verifies pipe closure stops the helper and lets stderr draining finish. Repeated race tests pass.
🤖 Comment by Codex on behalf of Greg.
| plus SIGHUP on Unix. Cancellation cleans up temporary files and preserves | ||
| existing output until publication starts. | ||
|
|
||
| ### Changed |
There was a problem hiding this comment.
The changelog has no entry for the change to nested uint128 values in JSON-encoded map and array columns.
With the earlier mmdbwriter version, mmdbtype.Uint128 was type Uint128 big.Int, which has no exported fields. json.Marshal therefore wrote nested uint128 values as {} in CSV and Parquet string columns. This branch writes them as exact decimal numbers. Users who parse these columns, or who worked around {}, get different output with no release note. A "Fixed" entry would cover it.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
This one seems possibly worth mentioning.
There was a problem hiding this comment.
I've never seen a uint128 in the wild, but I'll add it.
There was a problem hiding this comment.
Added the Fixed entry in c30641b: nested 128-bit unsigned integers in maps and arrays now serialize as exact JSON numbers in CSV and Parquet string columns.
🤖 Comment by Codex on behalf of Greg.
| "github.com/maxmind/mmdbwriter/v2/mmdbtype" | ||
| ) | ||
|
|
||
| func normalizeMMDBJSON(value mmdbtype.DataType) any { |
There was a problem hiding this comment.
This deep-copy walker exists only because mmdbtype.Uint128 has no json.Marshaler. That type is in mmdbwriter v2, which we own and which is not released yet. That is a better place for the fix.
Each Map or Slice column value on each row is copied into map[string]any/[]any with a new memo map. In a local benchmark, this allocated about 3 times as many bytes per value (738 B to 2205 B). The pointer/length/kind memo protects against cycles that MMDB decoding cannot make.
A MarshalJSON on mmdbtype.Uint128 that writes a decimal json.Number would remove json.go and jsonContainer. It would also fix JSON encoding for other mmdbwriter users.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Yeah, I considered this but didn't really think anyone else would be serializing it as JSON. I'll make a follow-up issue.
There was a problem hiding this comment.
Created mmdbwriter #272 for both parts: add value-receiver Uint128.MarshalJSON upstream, then update this dependency and remove the converter’s recursive normalization code. The follow-up retains exact-number and JSON-error regression coverage.
🤖 Comment by Codex on behalf of Greg.
There was a problem hiding this comment.
The internal issue is ENG-5506.
|
|
||
| // run performs the main conversion process. | ||
| func run(opts mmdbconvert.Options, logger *slog.Logger) error { | ||
| func run(ctx context.Context, opts mmdbconvert.Options, logger *slog.Logger) error { |
There was a problem hiding this comment.
Nit, follow-up to the earlier ordering thread: runCLI calls usage first (line 101), before extraConfigArgsMessage (line 139) and runConversion (line 165). But usage is still last in the file (line 256). By first-call order, usage goes before extraConfigArgsMessage.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Fixed in faa374b: usage now immediately follows runCLI, before extraConfigArgsMessage and runConversion.
🤖 Comment by Codex on behalf of Greg.
Tracking issue: STF-1809.
Interrupting a conversion currently skips deferred cleanup and can leave large staged CSV, Parquet, or MMDB files beside the output. Add cooperative cancellation that removes unpublished files and preserves existing output before publication begins. Once publication starts, finish the commit sequence.
This follows #115 and STF-1560 and is based on
main. The library changesRunto require a context without installing signal handlers. Existing Go callers must pass a context, such ascontext.Background()when cancellation is not needed. The CLI preserves inherited ignored SIGINT/SIGHUP, identifies handled signals, allows a second signal to terminate blocked cleanup, and restores native interruption behavior after cleanup on Unix and Windows.Update
mmdbwriter/v2tocc514d224474and adapt Uint128 conversions to its new value representation. Nested values in CSV and Parquet string columns encode as exact decimal JSON numbers. Low-word scalar conversions avoid big-integer allocations while preserving overflow checks and high-word float rounding.mmdbwriter #272 tracks adding upstream Uint128 JSON marshaling and then replacing this converter’s custom normalization with ordinary
json.Marshal.Acceptance criteria:
Run(ctx context.Context, opts Options) error. This is a breaking change; callers must pass a context. Library calls must not install signal handlers.nohupand SIGINT in background jobs. Do not subscribe to any candidate signal Go reports as ignored. Other platforms retain native signal handling without installing handlers; existing builds remain supported.errorfield and identify successful publication when conversion completes. Nonzero results do not imply that outputs were unpublished; unrelated parent cancellation is reported separately only when the callback is unsuccessful.128 + signal number.STATUS_CONTROL_C_EXIT(0xC000013A, signed-1073741510) after cleanup.Validation:
go test -race -count=1 ./...and scopedprecious lintpass atfaa374b.-race -count=10. Inherited ignored SIGINT/SIGHUP checks pass; Bash-dependent tests skip when Bash is unavailable.faa374b.Summary by CodeRabbit
Run; use a background context when cancellation is not needed.