Skip to content

Account for existing store usage when computing dynamic JetStream MaxStore - #8374

Open
marcusashmond wants to merge 1 commit into
nats-io:mainfrom
marcusashmond:fix/jetstream-maxstore-account-for-existing-usage
Open

Account for existing store usage when computing dynamic JetStream MaxStore#8374
marcusashmond wants to merge 1 commit into
nats-io:mainfrom
marcusashmond:fix/jetstream-maxstore-account-for-existing-usage

Conversation

@marcusashmond

Copy link
Copy Markdown

When JetStream runs in dynamic storage mode (no explicit max_file_store configured), the server computes MaxStore as roughly 75% of the currently free disk space on the store directory's filesystem. This calculation is redone on every server restart.

The problem: as JetStream writes data to disk over time, free disk space naturally drops, so the computed MaxStore ceiling also drops on each subsequent restart, even though nothing is actually misconfigured. In practice this means stream configs (sum of max_bytes) that were valid when first created can start failing to update or recreate after a restart, with insufficient storage resources available (err_code=10047), purely because the dynamic ceiling ratcheted downward.

This PR fixes the calculation by adding back the current on-disk size of the JetStream store directory before applying the 75% estimate, so a server that restarts with existing data ends up with a dynamic MaxStore roughly consistent with what it had before, rather than one that shrinks every time.

Resolves #8322

Related: #4140, which raised the same underlying issue with the calculation not accounting for existing usage.

Changes

  • Added a dirSize helper that walks a directory and sums file sizes, used to measure the current size of the JetStream store directory.
  • Updated the three platform implementations that perform a real filesystem-based calculation (disk_avail.go for Linux/macOS/BSD, disk_avail_openbsd.go, disk_avail_solaris.go) to add the store directory's current size back into available space before the 75% estimate.
  • Left disk_avail_windows.go, disk_avail_netbsd.go, and disk_avail_wasm.go untouched, since these already return a flat default rather than performing a real calculation.
  • Added TestJetStreamDiskAvailableAccountsForExistingStoreUsage, which writes data into a temp store directory and asserts the computed available space does not meaningfully drop as a result, reproducing the bug directly against diskAvailable before this fix.

Testing

Ran the new test in isolation and the broader TestJetStream suite locally to check for regressions in dynamic config behavior.

Note: while running the full test suite, TestJetStreamClusterSnapshotStreamAssetOnShutdown and TestJetStreamClusterStreamResetOnExpirationDuringPeerDownAndRestartWithLeaderChange failed once under load. Both pass reliably in isolation and on a clean checkout without this change, so this appears to be pre-existing flakiness unrelated to this fix.

…Store

Signed-off-by: Marcus Ashmond <marcusbrew18@gmail.com>
@marcusashmond
marcusashmond requested a review from a team as a code owner July 10, 2026 21:48

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 812b10ae4f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread server/disk_avail.go
if err := syscall.Statfs(storeDir, &fs); err == nil {
// Estimate 75% of available storage.
ba = int64(uint64(fs.Bavail) * uint64(fs.Bsize) / 4 * 3)
ba = int64((uint64(fs.Bavail)*uint64(fs.Bsize) + uint64(dirSize(storeDir))) / 4 * 3)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude orphaned snapshot staging from dynamic storage sizing

When dynamic JetStream storage starts after a failed snapshot/restore, .snap-staging can still exist under js.config.StoreDir; this size is added here before startup later removes that directory (jetstream.go:1230). In that scenario the computed MaxStore is inflated by 75% of transient data that is immediately deleted, so the server can accept stream reservations beyond the intended disk budget. Please skip snapStagingDir (or clean it before this calculation) when adding existing store usage.

Useful? React with 👍 / 👎.

@neilalexander

Copy link
Copy Markdown
Member

@claude review

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @neilalexander's task in 2m 38s —— View job


Code Review

  • Gathering context
  • Reading changed files
  • Analyzing correctness and logic
  • Reviewing test coverage
  • Posting final review

The fix correctly identifies the root cause and the mathematical approach is sound. Here are my findings, from most to least significant:


dirSize is duplicated across three files

The function is byte-for-byte identical in disk_avail.go, disk_avail_openbsd.go, and disk_avail_solaris.go. Since it only uses os and path/filepath (both cross-platform), it belongs in a single shared file with no build constraints — e.g., disk_avail_common.go. Any future bugfix or behavior change would otherwise need to be applied three times.

Fix this →


.snap-staging inflates the estimate after an interrupted restore

The Codex inline comment is correct. The call sequence is:

  1. EnableJetStreamdynJetStreamConfigdiskAvailable (calls dirSize(storeDir)) — jetstream.go:2726
  2. os.RemoveAll(filepath.Join(js.config.StoreDir, snapStagingDir))jetstream.go:1230

If a snapshot restore was interrupted, .snap-staging can still exist under storeDir and its size is included in the dirSize total before that directory is cleaned up. This inflates MaxStore by 75% of whatever data was in staging — effectively giving the server more headroom than the underlying disk justifies. The effect is permissive (more storage allowed), not restrictive, so it won't cause 10047 errors. But it can let reservations exceed the real disk budget.

The simplest fix is to skip the staging directory in dirSize, since that dir has a known name:

func dirSize(path string) int64 {
    var size int64
    filepath.Walk(path, func(p string, info os.FileInfo, err error) error {
        if err != nil {
            return nil
        }
        if info.IsDir() && info.Name() == snapStagingDir {
            return filepath.SkipDir
        }
        if !info.IsDir() {
            size += info.Size()
        }
        return nil
    })
    return size
}

Fix this →


Startup performance: filepath.Walk on large stores

On a server with many streams and years of data, the store directory can contain thousands or tens of thousands of files. filepath.Walk at startup is linear in the number of filesystem entries, and this happens synchronously during EnableJetStream. For very large stores this could add meaningful latency to restarts. The PR doesn't address this, but it's worth noting and potentially logging a warning or a duration.


Formula correctness (looks good)

For reference, the math checks out:

  • Prior to any writes: freeOrig disk available. MaxStore = 0.75 * freeOrig.
  • After writing N bytes of JetStream data: freeNow = freeOrig - N, and dirSize = N.
  • New formula gives: 0.75 * (freeNow + N) = 0.75 * freeOrig. ✓

The ceiling is stable across restarts, which is the intended invariant.


Test coverage

TestJetStreamDiskAvailableAccountsForExistingStoreUsage is well-structured and tests directly against diskAvailable rather than through a full server. It catches the regression clearly. One small note: on stub platforms (Windows, NetBSD, WASM), diskAvailable ignores storeDir entirely and returns JetStreamMaxStoreDefault unconditionally, so the test trivially passes there without exercising the logic. This is acceptable since the fix is platform-specific, but a comment in the test acknowledging this might help future readers.


Minor: casting int64 to uint64 for dirSize result

dirSize returns int64 and the expression uses uint64(dirSize(storeDir)). While dirSize won't return a negative value in practice (only file sizes are summed), the silent cast from a signed to unsigned type could theoretically wrap around if something unexpected happened. A guard like if sz := dirSize(storeDir); sz > 0 { ... } before the cast would make the intent explicit.


Overall: The core fix is correct and the test validates it well. The two issues worth addressing before merge are the dirSize duplication and the .snap-staging inflation edge case. The rest are lower priority.

@neilalexander

Copy link
Copy Markdown
Member

@claude check #8322 (comment)

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @neilalexander's task in 1m 51s —— View job


Analysis: issue #8322 comment by @MauriceVanVeen


The referenced comment is from @MauriceVanVeen (a NATS team member), posted on 2026-06-19 in the original bug report. The full text:

An improvement in this area would be welcome! 🙂

Restoring the streams first, and then adding back the reported bytes to the dynamic limit would at least ensure that the server can still start up, even if the overall dynamic mode is still best-effort.

How this differs from the PR's approach

The team's suggestion and the PR's implementation take meaningfully different approaches:

Team suggestion (MauriceVanVeen) This PR
When measurement happens After stream recovery Before stream recovery (dynJetStreamConfig is called at EnableJetStream:211, before Account.EnableJetStream runs)
What is measured NATS-reported stream bytes (internal accounting) Raw filesystem bytes via filepath.Walk (dirSize)
Cleanup of .snap-staging Already removed (line 1230 runs before recovery completes) Not removed yet — inflates the estimate
Includes orphaned/temp files No — only committed stream data Yes — everything under storeDir

The suggested flow was: recover streams → read their reported usage → add that back to the dynamic limit. This is a two-phase approach where MaxStore starts permissive, streams load, and then the ceiling is tightened/adjusted to reflect reality based on what NATS itself knows it has stored.

The PR's approach — measure disk usage at dynJetStreamConfig time and use it to inflate fs.Bavail — is architecturally earlier and uses a coarser signal (raw filesystem bytes rather than NATS internal accounting). It happens at jetstream.go:2726, well before stream recovery, and before .snap-staging is cleaned up at line 1230.

Practical implications

The PR does fix the ratchet-down bug correctly (the math checks out), but the approach has the .snap-staging inflation edge case raised in the Codex review, which would not be present in MauriceVanVeen's suggested approach. It also includes orphaned/compaction files in the tally, which NATS's own stream reporting would not.

One tradeoff worth noting: MauriceVanVeen's suggestion requires that stream recovery itself doesn't fail due to MaxStore being too small — but since error 10047 only fires on stream create/update, not on recovery of existing state, this should be safe. Recovery would complete normally, and then MaxStore could be adjusted upward based on the actual recovered stream sizes.

The PR as written is simpler and solves the main problem, but @neilalexander / the team may want to consider whether the implementation should more closely follow the two-phase approach described in the issue comment (recover first, then adjust MaxStore using internal stream byte reporting rather than dirSize). That approach would naturally avoid both the .snap-staging inflation issue and the filesystem-walk-at-startup performance concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants