Fix/lifecycle and safety bugs#88
Open
Foxof7207 wants to merge 7 commits into
Open
Conversation
cString returned a raw uintptr into unpinned Go memory — invisible to the GC, so libgeyserlite could observe the backing array moved or reclaimed before geyser_init read it (fails under checkptr, memory pressure, or a moving GC). Replace it with pinString, which runtime.Pinner.Pin's the array and only Unpins after the native call returns. The cleanup also runtime.KeepAlive's the slice so the GC never sees it as garbage between Pin and Unpin. Behavioral change: the pointer handed to native code is now genuinely pinned for the duration of the call, instead of relying on the slice being reachable through an incidental capture. No public API change (cString was unexported). Adds embedded_helpers_test.go covering the pin/release contract and the idempotent-release invariant; verified under -gcflags=all=-d=checkptr=2 and go test -race.
…task 2)
Both Go and Rust embedded runners called os.Chdir/set_current_dir into
the workdir so Geyser's relative-path file lookups would find
permissions.yml, key.bin, etc. CWD is process-global, not
goroutine-local, so this raced with any concurrent relative-path I/O
in the host and prevented running multiple embedded servers safely.
Fix: patch Geyser's getConfigFolder() (in apply-overlay.sh) to return
the config file's parent directory instead of System.getProperty("user.dir").
This makes permissions.yml, cache files, and any relative resource resolve
relative to where config.yml lives — matching how getSavedUserLoginsFolder()
already worked. The config path passed to geyser_init is already absolute
(via filepath.Join(workdir, "config.yml")), so no CWD dependency remains.
Subprocess mode is unchanged: it sets cmd.Dir / .current_dir() which is
child-process-local and concurrency-safe by design.
Removes CdGuard from Rust embedded.rs (no longer needed).
…(tasks 3,4,8) Server lifecycle (task 3): - started is now reset to false when run() returns, so Start can be called again after completion (server is reusable, not single-use). - A fresh done channel is created per Start, preventing close-on-closed. - The run error is stored before teardown so Err() returns it correctly. Stop semantics (task 4): - Stop() now returns nil on successful shutdown instead of leaking the run error (context.Canceled, crash error, etc.). - Returns nil if already stopped (not running) — idempotent. - Returns ctx.Err() only if the stop context expires before shutdown. - Adds Err() accessor for callers who need the last run error. - Removed ErrNotStarted sentinel (no longer returned by Stop). Rust parity: - Rust Server.start() now resets the started flag after the run completes, matching Go behavior and the GeyserBridge native side. Healthy docs (task 8): - Documented Healthy() as an eventually consistent signal. Started flag may flip between the atomic read and the runner health check during teardown; false is authoritative, true is best-effort.
…(tasks 5,6) Backoff reset (task 5): - Go subprocess supervisor now calls backoff.reset() when a run lasted >= stableRunThreshold (5 minutes, matching Rust). Previously the reset() method existed but was never called, so once max backoff was reached it stayed poisoned across unrelated incidents. - Short crash loops still escalate backoff normally; only stable runs reset it. Process group signaling (task 6): - Go: cmd.Cancel now signals the entire process group via kill(-pid) on Unix instead of just the direct child. Geyser-spawned grandchildren (workers holding the UDP port) are now terminated on shutdown. - Rust: send_sigterm targets the process group (negative pid) too, matching Go. ESRCH (already exited) is treated as success. - Non-Unix builds fall back to signaling the direct child; no brittle cross-platform process-tree hacks introduced. - Handles already-exited processes (os.ErrProcessDone / ESRCH) cleanly. Adds tests for stableRunThreshold consistency with Rust and backoff reset-after-stable-run behavior.
splitHostPort previously fell back to the default port on any parse error (non-numeric port, out-of-range port, malformed string), so "127.0.0.1:abc" silently started on the default port instead of failing. Invalid configs could bind the wrong port. Replaced with a validating parser (Go: splitHostPort returns error; Rust: split_host_port returns Result) that: - rejects non-numeric ports - rejects ports outside 1..=65535 - rejects malformed host:port strings and unclosed IPv6 brackets - preserves supported shorthand: "", ":port", bare host, "[::1]" Both Go Options.validate() and Rust Options::validated() now reject invalid Listen/Upstream before Start, so misconfiguration fails loudly. Behavioral change: previously-accepted invalid endpoints now error. This is intentional — silent fallback to defaults on malformed input was the bug. Adds table-driven tests for valid shorthand, IPv6, and the full set of invalid cases in both Go (config_test.go, geyserlite_test.go) and Rust (config.rs).
…9,10) Exit error reporting (task 9): - Extract formatExitError helper that distinguishes signal death from ordinary exit codes. Previously exec.ExitError.ExitCode() returned -1 for signal kills, producing misleading "subprocess exited -1". - On Unix, signalFromExitError extracts syscall.WaitStatus to surface the actual signal name (e.g. "killed by signal killed"). - Ordinary nonzero exits still report their code accurately. - Adds exit_error_test.go with real subprocess tests. ANSI stripper (task 10): - Replaced hand-rolled byte scanner (only matched 'm' terminator) with a correct CSI regex in Go: matches all final bytes in 0x40..0x7e (m, K, J, H, etc.). Truncated sequences left intact. - Fixed Rust strip_ansi to match: now handles all CSI final bytes and preserves truncated sequences (ESC[ without a final byte). - Both Go and Rust strippers are consistent: same behavior for SGR, erase, cursor-position, and truncated sequences. - Adds table-driven tests in both languages.
…ion) - Collapse nested if-let in paths.rs (clippy collapsible_if). - Apply cargo fmt to config.rs and subprocess.rs.
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
1 participant
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.
See more details in each commits