Repository navigation
Conversation
Include expansion had no ceiling on its result. The same file may legally be included many times (only active recursion is blocked), so a 1 MB config can amplify into tens of megabytes of expanded text. Add an expansion budget covering expanded bytes and lines, per-file bytes, file count, nesting depth, and total source bytes, and stop the walk at truncation rather than continuing to read files whose output is discarded. Replace globSync with a segment-by-segment directory walk that retains only the 256-match cap instead of materializing every match first, and refuse patterns deeper than 64 segments before walking them. Re-check the per-file ceiling against the decoded string: invalid UTF-8 decodes to U+FFFD at 3 bytes out per byte in, so a 400 KiB file passed the 1 MiB stat check and landed as 1.2 MB of text. The walk now matches POSIX glob(3), which is what ssh itself uses to resolve Include: `**` is two wildcards in one segment rather than a recursive descent, and symlinked directories are traversed. globSync differed on both.
* fix(skills): require updater registration * fix(skills): reject incomplete updater registrations --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
* fix(quality): clear safe existing lint findings * fix(quality): keep lint cleanup allocation-free * fix(quality): enforce performance-safe baseline * test(terminal): drain deferred confirmation cleanup
The SSH config include ceilings drop configuration when a limit is hit — a host defined past the ceiling simply never appears. `loadUserSshConfig()` returned the surviving prefix as an ordinary array, so the import surface reported "Synced N servers" for a partial import and the only signal was a `console.warn` the user never sees. Thread a typed reason from the budget through the expander, parser, store, and IPC to both renderer import surfaces, which now warn rather than claim success. Every drop path records; a repeat drop still registers even though the warning itself is deduped. The ceilings themselves are unchanged — this reports what they already do. Co-authored-by: Orca <help@stably.ai>
Refresh the persisted Windows PATH during preflight without blocking Electron's main thread. Bound and deduplicate registry reads, preserve the last good cache on failure, skip host refresh for WSL, and add Windows regression coverage.
# Conflicts: # src/cli/handlers/agent-hooks.ts # src/cli/runtime/transport.ts # src/shared/ephemeral-vm-recipes.ts
10 tasks done
Collaborator
|
Still valid on main: |
This branch has not been deployed
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.
Slice 8 of the re-landed OOM work. Base is
oom-slice-7-ssh-fs(#10878) — review only the top commits.ELI5
Your
~/.ssh/configcan sayInclude conf.d/*.conf, and those files can include more files. Orca reads that whole tree into one big string before parsing it.Nothing capped the result. Three ways that gets large:
Include conf.d/*.confcalledglobSync, which builds an array of every match before anything is capped. The 256-match cap was applied after the full list already existed.Now expansion runs against a budget, globbing walks the tree one directory at a time and only ever retains 256 matches, and the size check also runs against the decoded result.
And when a ceiling does drop part of your config, the import says so. That second part is the follow-up commit; see below.
What changed
Expansion budget (
ssh-config-expansion-budget.ts, new)Bounded globbing (
ssh-config-bounded-glob.ts, new)globSyncwith a segment-by-segmentopendirSyncwalk that inserts into a sorted array capped at 256. Peak retention is the cap, not the match count. The directory handle closes on every exit path.Truncation is reported to the user (follow-up commit)
toast.warningnaming the ceiling instead oftoast.success("Synced N servers"). See "The bug this second commit fixes" below.Proof the OOM fix works
Every bound was mutation-tested — the bound is removed and the
src/main/sshsuite re-run. All 12 fail:expandedBytesexpandedLinesfileBytesfilesglobMatchesnestingDepthsourceBytesMAX_GLOB_PATTERN_SEGMENTSThe segment cap is load-bearing beyond memory. Removing it doesn't fail a test, it hangs: the 65-wildcard pattern in that test roots at
/and becomes a full filesystem walk to depth 65. Killed at 120s on the file alone and at 14 minutes on the full suite.Four coverage gaps this found. The first three were vacuous in the same way, and are the reason this slice's tests changed:
expandedBytesandexpandedLinescould each be raised toMAX_SAFE_INTEGERwith the suite green. The tests seeded the budget relative to the constant (expandedBytes - 1), so the assertion held at any value — including one that is no bound at all. Replaced with two tests that drive the real expander and assert against the literal ceiling.The bug this second commit fixes
Review of the first commit surfaced a defect in the bounds themselves, so it's fixed here rather than left as an accepted regression.
What went wrong. The ceilings drop configuration — that's their job — but
loadUserSshConfig()returned the surviving prefix as an ordinarySshConfigHost[]. Every caller treated it as the complete config. So:Host prod-dbdefined inconf.d/40-databases.conf.prod-dbis simply absent.console.warnin the main process, which no user sees.A host that Orca silently declined to read is indistinguishable from a host you never configured. That's a worse failure than the OOM: the crash is loud, this is silent and looks correct.
Confirmed as introduced by this slice, not pre-existing. At the merge base (
f10b6de2c7),ssh-config-expansion-budget.tsandssh-config-bounded-glob.tsdo not exist, and the expander had only a glob-match cap and a per-file size cap. The four ceilings that can drop whole host definitions — nesting depth, expanded output, source bytes, file count — are all introduced by this slice.The fix.
SshConfigTruncationReasonis declared once insrc/shared/ssh-types.tsand threaded end to end:Details worth a reviewer's attention:
warnOncelogs a given ceiling once, but a second drop from the same ceiling still registers, so the reason set is complete by construction.Mutation-tested, 9 mutations, all fail:
warnOncestops recording the reasontruncatedBy: nullThe end-to-end test drives
loadUserSshConfig()itself with a real config on disk rather than calling the bounded helper directly — deleting any link in the chain above fails it. That's deliberate: a test that calls the helper directly stays green when the wiring is removed.A semantic difference found by differential testing
The old and new resolvers were compared across 22 include patterns over a fixture tree (nested dirs, hidden files, spaces, bracket/question-mark literals, a symlinked directory). Four diverged — all four because Node's
globSyncand the ssh client'sglob(3)genuinely disagree:globSyncglob(3)conf.d/**/*.conf**/*.conf*/*.conf*/*/*.confThe third column is the shell's own expansion with
globstaroff, i.e. plain POSIXglob(3)— the same semanticssshuses to resolveInclude. It matches this PR on all four. So**is two wildcards in one segment rather than a recursive descent, and a symlinked directory is traversed. Under the old behavior Orca could read include files thatsshwould not, and skip ones it would. Both cases are now pinned by tests.This is a real behavior change, called out below.
Proof of zero regressions
src/main/ssh+src/main/providers+src/shared— 4,787 passed, 0 failed (16 skipped).origin/main):src/main/ssh+src/main/ipc/ssh.test.ts+ the new renderer lib test + the persistence tests — 1,495 passed, 0 failed (10 skipped).tsconfig.nodeandtsconfig.web.loadUserSshConfig()'s return type changed fromSshConfigHost[]toSshConfigLoadResult, so every call site and its tests were updated; the type checker enumerates them exhaustively.SshPane.tsxcrossed the 400-linemax-lineslimit as a result, so the two toast reporters were extracted tossh-config-import-report.tsrather than disabling the rule.User-facing behavior changes
Includepatterns now resolve the waysshresolves them.**no longer descends recursively, and symlinked directories are now traversed. Anyone whose config relies on**recursing will see fewer files included — butsshwas already not reading those files, so the config was already not behaving as it appeared to in Orca. Nothing in this repo uses**in anInclude.A config tree expanding past 16 MiB or 200,000 lines is truncated — and the import now warns you that hosts were skipped, rather than reporting success. Reached only by pathological amplification; an ordinary config with a
conf.ddirectory is nowhere near it.An include matching more than 256 files processes the first 256 sorted matches and warns. The cap already existed; it's now applied during the walk rather than after materializing every match, and it now surfaces in the UI.
A partial import raises a warning toast naming the ceiling instead of a success toast. New copy, added to
en.jsononly, matching how strings are added elsewhere in the repo.Made with Orca 🐋