[IMPROVED] Index stream sources to avoid backward scans

[MauriceVanVeen]

The first commit refactors fs.recoverTTLState() and fs.recoverMsgSchedulingState(), since if both required recovering that would result in two separate linear scans, instead of one linear scan reused for recovering both.

The second commit introduces indexing for stream sources, which will ensure that backward scanning for pre-existing stream sources is eliminated.

• As messages come in, their Nats-Stream-Source is inspected and the source key is mapped to the sequence that's sourced.
• That state is kept in-memory and can be reused by methods like mset.startingSequenceForSources() to use the index, and only fall back to backward scanning remaining sources if necessary.
• This index is written to disk as sources.db when using file-based streams, similar to the thw.db and sched.db files.

The approach of having an index that scales by sources versus that scans the stream by messages results in a tremendous performance improvement, especially as streams get larger and the backward scan would otherwise need to reach way further back.

The sources.db file is stored at the stream's root, as opposed to in the msgs directory, since it should not be removed if the stream is purged:

  streams/<stream>/
  ├── meta.inf, meta.sum,meta.key
+ ├── sources.db
  ├── msgs/
  │   ├── 1.blk, 2.blk, …
  │   ├── index.db, thw.db, sched.db
  └── obs/
      └── <consumer>/…

The third commit adds a check preventing users from publishing messages with the Nats-Stream-Source header directly. And the fourth commit fixes a test that was (incorrectly) checking pre-2.10 coverage.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 9m 36s —— View job

---

PR Review

• Gather context
• Review commit 1: TTL/schedule recovery refactor
• Review commit 2: Stream source indexing
• Post inline comments
• Final summary

---

Commit 1 — Combined TTL/schedule recovery scan

The refactor is clean. Replacing two independent linear scans with a single unified pass is a straightforward correctness win. The new recoverPerMessageState() correctly guards each per-message recovery type with its own seq >= xSeq filter inside the loop, so the code that was already correct is preserved exactly. Block-level skip optimisations (mb.ttls == 0, mb.schedules == 0) are preserved.

One semantic change to be aware of: I/O errors reading the TTL or scheduling state files during UpdateConfig previously propagated to the caller as an error (aborting the config update). They now log a warn and continue. For the initial startup path this was already the behaviour, so it's now consistent across both call sites — but it is a change. See inline comment on line 695.

Commit 2 — Stream source indexing

The design is sound. Indexing iName → last_sseq in memory and persisting it as sources.db mirrors the existing TTL (thw.db) and scheduling (sched.db) patterns faithfully. The file format is simple and the decode validates every field incrementally. The backward-scan fallback in recoverSourcesBackwardScan (and the existing startingSequenceForSources) ensures correctness even when the index is absent or stale.

No correctness bugs found. Issues flagged inline are all low/minor severity:

#	Severity	File	Summary
1	Minor	filestore.go:12024	const headerLen = 17 inside writeSourcesState() shadows the package-level constant from scheduler.go — redundant, same value, harmless
2	Low	filestore.go:2247	sourcesBlock can never skip an empty block: no per-block sources counter exists (unlike mb.ttls/mb.schedules) — worth a comment, future improvement
3	Low	filestore.go:2284–2295	Pre-2.10 source headers (indexName == "") are silently skipped in the combined forward scan (but correctly handled in the backward-scan path). Safe in practice (no sources.db can exist on a pre-2.10 stream), worth a comment
4	Low	filestore.go:695	recoverPerMessageState() always returns nil — the error return path in UpdateConfig is dead code; see semantic-change note above
5	Minor	filestore.go:12073	decodeSourcesState trusts count from the file without capping it — safe (loop exits on first EOF), but a sanity cap against remaining buffer size would be cleaner

The performance improvement is well-motivated: the old backward scan's cost was proportional to the depth of the stream, while the new index scales only with the number of distinct sources. For large streams this can be orders of magnitude cheaper.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74fe1f50ff

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cba650b81e

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d489ae5bd

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11fbc5f828

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27f6d1415e

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[MauriceVanVeen]

@claude, re-review.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 7m 38s —— View job

---

Re-Review

• Gather context
• Review commit 1: TTL/schedule recovery refactor
• Review commit 2: Stream source indexing
• Review commit 3: Non-sourced publish prevention
• Review commit 4: Test fix
• Evaluate Codex review findings
• Post final summary

---

Status of Previous Review Findings

Finding	Status
const headerLen = 17 function-local shadowing	✅ Fixed — renamed to package-level sourcesHeaderLen
recoverPerMessageState() always returns nil (dead error path)	⚠️ Still present — minor
No per-block source counter (combined scan can't skip empty blocks)	⚠️ Still present — minor, accepted limitation
Pre-2.10 headers silently skipped in combined forward scan (no comment)	⚠️ Still present — low
decodeSourcesState no sanity cap on count	⚠️ Still present — low

---

Evaluation of Codex P2 Findings

"Apply persisted source positions before empty-stream return" (stream.go) — ✅ Addressed. In startingSequenceForSources, the SourcesState() lookup now happens before the state.Msgs == 0 early return. The purge-survive test (TestJetStreamSourcesStateResumesAfterPurge) covers this path. The separate setStartingSequenceForSources function still returns early on state.Msgs == 0, but this is intentional — at runtime si.sseq is already set from prior message processing, so preserving it (not resetting) is correct.

"Store source index outside purged message directory" (filestore.go) — ✅ Not an issue. sourcesStatePath() returns filepath.Join(fs.fcfg.StoreDir, sourcesStreamStateFile) — the store root, not msgDir. The Codex finding was based on an incorrect reading. The purge() function only moves msgDir; sources.db at the root is untouched.

"Point source-index tests at the store root" (filestore_test.go) — ✅ Not an issue. All test assertions use filepath.Join(dir, sourcesStreamStateFile) where dir is the TempDir (= StoreDir). The tests are already pointing at the correct root-level path.

"Validate source headers before indexing them" (filestore.go) — ✅ Addressed by commit 3. Both processJetStreamMsgWithBatch (stream.go:6628) and checkMsgHeadersPreClusteredProposal (jetstream_batching.go:615) now reject any non-sourced publish carrying a JSStreamSource header with a 400 / 10224 error. The attack surface described by Codex no longer exists.

"Include root source index in snapshots" (filestore.go:12041) — Author acknowledged: state can be rebuilt and the snapshot format is being replaced. Accepted.

---

New Finding: Test Regression in TestFileStoreNoPanicOnRecoverTTLWithCorruptBlocks

This is a medium-severity issue introduced by the refactor.

Before the refactor, the test called fs.recoverTTLState() directly. That function unconditionally creates a new thw.HashWheel and then scans the (corrupted) blocks — which is what the test was guarding against (no panic during scan).

After the refactor, the test calls fs.recoverPerMessageState(). Inside that function, TTL recovery only runs when allowMsgTTL && fs.ttls == nil. But newFileStoreWithCreated (called at test setup) already calls recoverPerMessageState() once, which sets fs.ttls = thw.NewHashWheel(). So when the test calls recoverPerMessageState() a second time, the condition fs.ttls == nil is false — the TTL recovery path is skipped entirely, and no scan of the corrupted blocks ever happens.

The test still passes, but it is now testing nothing meaningful about the panic regression it was named to guard.

Fix: Reset fs.ttls to nil before the second call so the recovery path is actually exercised:

// After corrupting the blocks:
fs.mu.Lock()
fs.ttls = nil
fs.mu.Unlock()

require_NoError(t, fs.recoverPerMessageState())

Fix this →

---

Commit 3 — Non-sourced publish prevention

The check is correct and placed in both the single-node path (stream.go:6628) and the pre-cluster-proposal path (jetstream_batching.go:615). The test TestJetStreamSourceHeaderNotAllowedIfNotSourced covers R1 and R3. The update to TestJetStreamSourceBasics correctly bypasses the client path by injecting the pre-2.10 message directly into the store (mset.store.StoreMsg), which is the right approach since a real pre-2.10 server would have written that header without going through the new check.

Commit 4 — Test fix

Straightforward. The pre-2.10 coverage is restored via direct store injection and the assertions correctly verify that only one new message (A.B) is sourced after the pre-existing A 1 header.

Overall Assessment

The design is sound and the implementation is correct. The Codex concerns were either already addressed or invalid. The only actionable finding is the hollow test in TestFileStoreNoPanicOnRecoverTTLWithCorruptBlocks — the regression guard is silently bypassed after the refactor.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a13b5e808b

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[MauriceVanVeen]

@claude, brief re-review.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 2m 56s —— View job

---

Brief Re-Review

• Gather context
• Check outstanding findings from previous review
• Evaluate new Codex finding (old-style source positions)
• Assess hollow test issue
• Post final summary

---

Status of All Previous Findings

Finding	Status
const headerLen = 17 function-local shadowing	✅ Fixed
Hollow test TestFileStoreNoPanicOnRecoverTTLWithCorruptBlocks	✅ Fixed — fs.ttls = nil is now reset before the second recoverPerMessageState() call, restoring the panic-regression guard
recoverPerMessageState() always returns nil (dead error path in UpdateConfig)	⚠️ Still present — minor, no action needed
No per-block source counter (combined scan can't skip source-empty blocks)	⚠️ Still present — accepted limitation
Pre-2.10 headers silently skipped in combined forward scan	⚠️ Still present — low severity, safe in practice
decodeSourcesState no sanity cap on count	⚠️ Still present — low severity, loop exits quickly on short buffer

---

New Codex Finding: Old-style Source Positions (2026-06-10)

The finding and author response both hold up on inspection. For a pre-2.10 upgraded stream:

1. recoverSourcesState() finds no sources.db → fs.sources stays nil
2. recoverPerMessageState seeds fs.sources with zeros (lines 2185–2199) then removes the file if all-zero (lines 2207–2215) — so the store-level index deliberately stays empty
3. startingSequenceForSources() in stream.go falls back to its own backward scan, which handles pre-2.10 headers correctly and sets si.sseq
4. writeSourcesState() skips writing when count == 0 (line 12065) — no sources.db is produced until new messages arrive

The "gap" Codex describes (purge before first new message → restart starts from seq 1) is identical to pre-PR behaviour — the old code also had no index and would find nothing on an empty stream. The new code adds no regression here; it simply defers index creation until new messages actually land. Author's response is correct.

---

Assessment

The only actionable finding from the previous round — the hollow test — has been fixed correctly. The remaining low/minor items are all style/defense-in-depth and do not affect correctness. The new Codex concern is a pre-existing behaviour, not a regression. The PR is in good shape.