Fix lint warnings and consolidate CI test steps - #4
Merged
Conversation
- Suppress unchecked fmt.Fprint errors in CLI spinner (writes to
terminal, errors not actionable)
- Use strings.SplitSeq for iterator-based splitting
- Replace manual testing.Short() checks with skipInShortMode helper
- Fix test assertions: assert.EqualValues→assert.Equal,
assert.True(errors.Is)→assert.ErrorIs
- Use json.NewEncoder for commit_version response instead of Sprintf
- Call IsReadyForSplitReads on ShardStatus (not nested ShardInfo)
- Access IndexConfig.Name directly (not via nested struct)
- Fix byte slice literals: []byte{0x00}→{0x00}
- Remove redundant blank lines and trailing newlines
- Consolidate separate E2E + PG test CI steps into one
- Fix env var placement in CLAUDE.md command example
- Update termite submodule
notifyPendingResolutions() accessed db.pdb without holding pdbMu, racing with Close() which sets db.pdb to nil. Capture the pointer under the read lock and bail early if nil, matching the existing pattern in Get(). The deferred RecoverPebbleClosed handles the case where Close() runs after the pointer is captured. Fixes SIGSEGV in TestE2E_OCC_ConcurrentRMW during cluster cleanup.
…ce on shutdown During test cleanup, Close() sets db.pdb to nil under pdbMu before closing the Pebble DB. Meanwhile, background goroutines (transaction recovery loop) and in-flight Raft apply operations (ResolveIntents, WriteIntent, etc.) can race with Close() and dereference the nil pointer, causing a SIGSEGV panic. Fix by capturing db.pdb under pdbMu.RLock() at function entry and using the local copy, following the existing pattern from Get(). If pdb is nil, return pebble.ErrClosed. If Close() runs concurrently after the pointer is captured, Pebble returns ErrClosed which RecoverPebbleClosed handles. Protected functions: - notifyPendingResolutions (background transaction recovery loop) - ResolveIntents (Raft apply path) - InitTransaction (Raft apply path) - WriteIntent (Raft apply path) - GetTimestamp (Raft apply path) - buildConflictingIntentsMap (called from WriteIntent) - hasConflictingIntentForKey (called from WriteIntent) - loadTxnRecord (called from finalizeTransaction) - finalizeTransaction (called from CommitTransaction/AbortTransaction)
The 6-line lock-read-unlock-nil-check pattern was repeated 12 times across db.go, helpers.go, and ttl.go. Extract it into a single getPDB() method on DBImpl that returns nil when the DB is closed. Also remove redundant inline comments that duplicated the helper's doc comment.
…ference on shutdown Comprehensive fix for the shutdown race where Close() sets pdb=nil while background goroutines or in-flight Raft apply operations still access it. Protected functions: SetRange, UpdateRange, SetSplitState, ClearSplitState, loadSplitState, SetSplitDeltaFinalSeq, ClearSplitDeltaFinalSeq, ListSplitDeltaEntriesAfter (CI crash site), ClearSplitDeltaEntries, finalizeSplitInternal, streamRangeToDB, Split, openIndex, FindSplitKey, Scan, Snapshot, expandFilterIDsForChunks, loadMetadata, saveMetadata, saveSchema, saveIndexes, shouldWriteValue, shouldWriteValueLegacy, AddEdge, DeleteEdge, UpdateEdgeWeight, getOutgoingEdges, collectOutgoingEdgeKeys, extractSpecialFields edge reconciliation, TTLCleaner, and EdgeTTLCleaner.
…ision The join planner cache key only included table names and field names, not the join type. An INNER JOIN followed by a LEFT JOIN on the same tables would reuse the cached INNER plan, silently dropping unmatched rows. Fix by serializing the full JoinClause (including JoinType) into the cache key. Add regression test and use SyncLevelFullText in the e2e join test for the late-inserted unmatched row.
ajroetker
pushed a commit
that referenced
this pull request
May 31, 2026
Address the remaining review findings on dynamic-template handling: - Overlapping-rules asymmetry (#4): ingest now stops at the first selector-matching rule (Elasticsearch order) instead of falling through to a later overlapping rule when a value fails to coerce — a non-coercible value simply yields no fact, like a static typed field. Query-time dynamicFieldConfig resolves a field only when all name/path-matching rules agree on the scalar type; on disagreement it declines (the aggregation falls back to a complete scan) rather than reading a type ingest may have stored differently. Together these guarantee query never reads a type ingest didn't store. New test covers the overlapping-disagreement case end to end. - Duplicated matcher: dynamicRuleMatches and dynamicRuleResolvesField are unified behind a single dynamicRuleSelectorMatches(rule, path, name, ?value) evaluator, so ingest and query selector semantics cannot drift. match_mapping_type is unsatisfiable when value is null (query time), and query additionally requires a name/path selector. - query_field aliasing: documented that dynamicFieldConfig's returned name/path alias the caller's query_field and that this is safe because every consumer is query-scoped (planner copies the name into owned canonical metadata tuples; resolveField already borrows query_field into .public). No code change needed. - reloadConfigJson concurrency: documented the invariant that the in-place config swap/free is only called under the provisioned write source's exclusive structural-mutation lock on a freshly-opened unshared DB handle (mirroring the adjacent core.setSchema swap), so it is not a use-after-free; added a guard note for any future shared-reader use. Investigation also surfaced a separate, pre-existing UAF unrelated to this work: the serverless and C-API aggregation paths free aggregation requests without the cloneSearchAggregationResultLabelsDeep that table_reads.zig performs, leaving result name/field/type borrows dangling. Not touched here; flagged for a dedicated fix.
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
1 participant
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.
Summary
testing.Short()skip blocks in E2E shard split tests with the existingskipInShortModehelperIsReadyForSplitReads()onShardStatusinstead of the nestedShardInfo(matching the method receiver)json.NewEncoderfor the commit_version response instead offmt.SprintfTest plan