Repository navigation
fix: match a sandbox name as a whole argv token, not a substring - #53
Merged
Merged
Conversation
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
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.
Motivation
Two questions were sharing one filter, and neither was answered correctly.
extract_open_ttysselected wrapper lines withline.contains(sandbox). Every identity-suffixed sandbox name has the legacy bare name as a prefix, andsandbox_common.sh::_is_reapable_namestill hands out that bare name for a pre-identity sandbox:Asking for
linera-agentreturned all three terminals. A session's own tty is always inside a superset, so!open.contains(tty)could never be true —terminal_is_gonefailed open forever for that one name, silently, and it is the name that keeps the oldest sandbox on the dashboard. #50 made that path load-bearing, which is what makes this worth fixing now rather than noting.But tightening that match alone would have silently disabled orphan reaping, because the other caller depends on the looseness:
scan_host_open_ttysasks withsandbox_name(), which falls back to the literal"linera-agent"wheneversbx lsdoes not show exactly one running sandbox — i.e. always, on a host running several.linera-agent-<hash>terminal, so it worked by accident. Matched precisely, it returns nothing.decide_action's Guard 1 (open_ttys.is_empty() && alive_count > 0) skips every pass reporting "probable host scan failure" — a feature that is off while blamingps. Guard 1 is the only reason this would not have mass-killed live sessions instead.The legacy naming scheme is unaffected either way: a genuinely old sandbox is named exactly
linera-agent, so itssbx execlines carry that as a whole token, and its registry slice is keyed by the same string (registry_sandbox_name→resolve_sandbox_name). Render and--restore-sbx-sessionsboth keep working. The substring behaviour only ever helped when the name asked about was a prefix of a different sandbox's name, which is never the question the render path wants.Change
Split the two questions:
open_host_ttys_by_sandbox)scan_host_open_ttys)host_open_ttys)extract_open_ttysmatches the name as a whole whitespace-delimited argv token.sbx exectakes the sandbox as its own bare argument, so token equality is exactly right — and strictly narrower than the substring test, so no row that was judged before stops being judged.scan_host_open_ttysdrops the name filter entirely, from the same singlepssweep. This is strictly better than the old substring behaviour, which missed a differently-named sandbox's terminals —SANDBOX_NAME=my-team-sandboxcontributed none, so its live sessions looked like orphans and were kill candidates. A host-wide set is also the conservative direction for a kill decision: a tty absent from a broader set is stronger evidence, never weaker.Fallout:
sandbox_name()lost its last caller and is deleted, along with the reaper's now-vestigialDEFAULT_SANDBOX_NAME(sandbox_registrykeeps its own for the resolver call that still needs one). No#[allow(dead_code)]. Losing that helper is a bonus — an "always answers, with a convention" name resolver is the exact footgun this bug is made of, andtarget_sandbox(), which refuses to invent a name, remains.Tests
the_legacy_bare_name_does_not_match_its_identity_suffixed_siblings— with all three names present, the bare name must own exactly/dev/ttys001, and a suffixed name must still resolve to its own terminal.the_orphan_reapers_host_scan_is_not_narrowed_by_any_sandbox_name— a sweep carrying a hashed name, the bare legacy name andmy-team-sandboxmust yield all three terminals, whileextract_open_ttys(ps, "my-team-sandbox")still yields exactly one. This is the case the original commit broke.extract_open_ttys_picks_sandbox_host_ttyandextract_open_ttys_honors_alternate_sandbox_nameunchanged and green — theirother-sandbox/my-team-sandboxcases were already distinct tokens.Verification
Review note
The second commit exists because the first was wrong on its own — the loose match was load-bearing for the orphan reaper, caught in review. Neither existing test found it: both callers were only ever exercised with real sandbox names, never with the convention
sandbox_name()invents when it cannot detect one.