feat(cmd): default db-max-handles to half of the process fd limit - #3919
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3919 +/- ##
==========================================
- Coverage 75.50% 75.32% -0.18%
==========================================
Files 450 451 +1
Lines 40464 40476 +12
==========================================
- Hits 30552 30490 -62
- Misses 7818 7886 +68
- Partials 2094 2100 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude can you review this PR? |
89fa1b9 to
1136052
Compare
|
Claude finished @infrmtcs's task in 7m 2s —— View job Review: default
|
1136052 to
57f31ce
Compare
|
Claude finished @infrmtcs's task in 8m 40s —— View job Re-review: default
|
57f31ce to
9bc43ae
Compare
|
Claude finished @infrmtcs's task in 5m 5s —— View job Re-review: default
|
| Item | Author's position |
|---|---|
Ceiling magnitude (1_048_576) |
Deliberate — never-evicting is desirable |
No //go:build unix on utils/fdlimit.go |
Deliberate — only linux/darwin are supported targets |
1024/1048576 duplicated across four sites |
Known trade-off; generator regex + package boundary make the alternatives worse |
Those are all judgment calls in your area, and you've now reaffirmed each — treating them as settled and not re-raising them. One factual footnote on the first, for the record rather than as a request: pebble's MaxOpenFiles is a file-cache eviction limit, not RocksDB's max_open_files (which additionally gates index/filter preloading), so the two knobs aren't quite the same lever — but the shape of the decision (unbounded-by-default, derived from the ulimit) is well-precedented, and the "min 1024, max 1048576" disclosure in the usage string and docs means an operator can see what they're getting. Your call, and it's documented.
Fresh checks on this head
Since the content is unchanged I re-verified the parts most likely to break silently rather than re-reading what I'd already confirmed:
- Only one consumer.
cfg.DBMaxHandlesreaches exactly onepebblev2.Newcall (node/node.go:221) — no second store that would double-count the fd budget, and nothing anywhere comparesDBMaxHandlesagainst the old1024to infer "unset", so making the registered default machine-dependent can't perturb precedence logic. - No later
RLIMIT_NOFILEmutation. The onlySetrlimitcalls in the tree areRLIMIT_CPU/RLIMIT_ASinstarknet/compiler/rlimit_linux.go, so nothing raises the nofile limit afterNewCmdcomputes the default — the value read at flag-registration time is the one that stays true. - Package layout is right.
juno_internal_test.goaspackage mainsits correctly alongside the existingpackage main_testfiles (compile_test.go,dbcmd_test.go,usage_test.gofollow the same split), so the unexporteddbMaxHandlesForFDLimitanddefaultMaxHandlesare reachable without exporting anything. - All 9 table cases re-checked by hand, and the typing argument holds:
min(fdLimit/2, dbMaxHandlesCeiling)evaluates inuint64(untyped const converts up), result is always in[1024, 1_048_576], soint(...)cannot truncate even on a 32-bit target.
Verified
golangci-lint ✅ and Dependency Review ✅ are green on this head. go vet ./utils/ ./cmd/juno/ is clean locally — that type-checks the new test files too, so TestDBMaxHandlesForFDLimit and the reworked TestConfigPrecedence at least compile. I still could not run go test (command not permitted in this environment), and Juno Test was in progress when I reviewed, so execution of the new tests remains unverified on my side — worth a glance at that run before merging. Given the rebase, though, this content already passed a full CI cycle on the previous head.
· branch dat/fd-limit-default
9bc43ae to
8508057
Compare
|
Claude finished @infrmtcs's task in 7m 3s —— View job Re-review: default
|
8508057 to
61967c3
Compare
|
Claude finished @infrmtcs's task in 10m 8s —— View job Re-review: default
|
61967c3 to
20cd3ca
Compare
|
Claude finished @infrmtcs's task in 6m 59s —— View job Re-review: default
|
dbCacheSizeUsage at |
|
|---|---|
previously reviewed head (based on e89a546d5) |
234 |
the merge commit actions/checkout actually linted |
237 |
--new-from-patch was fed pull.patch in head coordinates, where this PR's added region was 235–239 — so merge-line 237 landed inside it and a 116-char line the PR never touched got classified as new. That's the coordinate drift I guessed at last round, confirmed with the real numbers (and it also means my "the reported line doesn't match this head" phrasing was backwards — 237 was correct for the checkout being linted, just not for the branch). Now that the branch sits on afd490f8d, head and merge coordinates agree, the added range is 238–242, line 237 is a context line, and the step passes. No change needed; wrapping dbCacheSizeUsage remains optional insurance if it ever recurs.
Closing out the Getrlimit question — fully settled, and Cur is the better choice
I raised this on Aug 11 and partially answered it last round from rlimit.go alone. I can now close it properly: grepping the whole syscall package on the Go 1.26 toolchain source (go.mod says go 1.26.0) shows origRlimitNofile is read in exactly four places — exec_unix.go:286, exec_linux.go:261, the other exec_* variants, and tests — and written only by init, by exported Setrlimit, and by Prlimit-on-self (syscall_linux.go:1314). Getrlimit never consults it. So MaxFDLimit() sees the raised soft limit and the feature is not a no-op on soft=1024, hard=1048576 hosts.
Better still, reading Cur turns out to be more correct than the lim.Max I suggested: rlimit.go:45 ignores the error from setrlimit, and on darwin adjustFileLimit clamps the raise to kern.maxfilesperproc. If the raise fails or gets capped, Getrlimit reports what the kernel actually enforces, whereas Max would over-promise. Your original choice is the right one — withdrawn on both counts.
Fresh checks on this head
- Rebase carried cleanly.
git diff origin/main...HEADis exactly the seven feature files; main's newdisable-syncflag didn't leavejuno_test.go's expected-config tables stale on this branch. - Still a single consumer.
cfg.DBMaxHandles→pebblev2.WithMaxOpenFilesatnode/node.go:243only. The otherpebblev2.Newcall sites (cmd/juno/dbcmd.go:364, and the tests) pass noMaxOpenFiles, so DB subcommands keep pebble's own default and can't double-count the fd budget. go vet ./utils/ ./cmd/juno/is clean locally (this type-checks both new test files).
Everything from rounds 1–5 stands as recorded: ceiling clamp, hermetic table test, un-exported defaultDBMaxHandles, Floor/Ceiling rename, corrected docstring, regenerated docs row. The build-tag scope and the duplicated 1024/1048576 literals are settled as deliberate, and the remaining defaultDBMaxHandlesFloor naming nit is @EgeCaner's call.
Verified
golangci-lint ✅, CodeQL ✅, Dependency Review ✅, Test Documentation Build ✅, Code Quality ✅ on this head. Juno Test, Container Security and Deploy and Test on Dev were still in progress when I finished — and go test is not permitted in this environment, so execution of TestDBMaxHandlesForFDLimit, TestMaxFDLimit and the reworked TestConfigPrecedence is still unverified on my side; worth a glance at Juno Test before merging, especially since it's the first run of these tests on a base whose juno.go changed. I re-checked all 9 table cases by hand and the typing argument holds — min(fdLimit/2, defaultDBMaxHandlesCeiling) evaluates in uint64, result always in [1024, 1_048_576], so int(...) cannot truncate.
· branch dat/fd-limit-default
Summary
db-max-handlesto half the process's open file descriptor limit instead of a fixed valueutilshelper to read the fd limit, with a testcmd/juno