fix(storage): shard single-writer commits by key hash

[matthyx]

Summary

The single-writer write path (#393, default-on since v0.0.331) funnels every Create/GuaranteedUpdate/SaveContainerProfile commit for all keys through exactly one goroutine per StorageImpl. That collapsed write concurrency from the legacy per-key-lock path's ~SqlitePoolSize-way parallelism (default 10) down to strictly 1-way for the entire resource kind.

This is a measured, confirmed regression: kubescape/node-agent's CI (a write-heavy, high-key-cardinality workload — many ContainerProfile objects for many pods written near-concurrently) went from 0-1 flaky test failures to 18-19 of ~30 test-suite jobs failing after the storage image bump that picked up v0.0.331, every one hanging on context deadline exceeded against the storage aggregated apiserver, sustained for the full ~19-minute test timeout. containerprofiles gets its own dedicated StorageImpl/writer instance (pkg/apiserver/apiserver.go), and 100% of the timeouts traced back to that endpoint specifically. This repo's own docs already flagged the risk before #393 flipped the default: docs/features/single-writer-priority-queue.md's "Known gaps" section said plainly "No throughput ceiling has been measured under deliberate load."

Fix

Correctness only ever required one in-flight committer per key — commit() already re-acquires the existing per-key lock and does an authoritative resourceVersion compare-and-commit specifically because writes to different keys touch disjoint metadata rows and payload files. A single global writer goroutine was therefore stricter than the design actually needed.

This PR routes each commit job to one of N shard goroutines (config.Config.SingleWriterShards, default 8, tunable) by hashing the key (FNV-1a, pure/deterministic, no allocation). Same key → same shard → same serialization guarantee as before (no-lost-updates, torn-read prevention, delete-race conflict detection — all unchanged, verified by the full existing test suite still passing unmodified except one deliberately-adjusted test, see below). Different keys that land on different shards now commit in parallel again.

commit() itself is untouched — only the routing above it changed.

Deliberately weakened, and why that's fine: highBurstLimit's high-vs-low fairness arbitration becomes per-shard rather than global. Jobs on different shards never competed for a goroutine's time in the old design either — there's nothing to arbitrate between jobs that were never contending. Only same-shard cross-priority contention is affected; within a shard the guarantee is exactly what it was before.

Changes

• pkg/registry/file/singlewriter.go — singleWriter becomes a router over N writerShards, each with its own high/low channels and its own goroutine running the same fairness loop. New DefaultSingleWriterShards (8), singleWriterShards, SetSingleWriterShards, shardIndexFor (pure FNV-1a hash → shard index), queueDepth (sums shard depths for the existing per-priority metric so it doesn't flap between shards).
• pkg/config/config.go — SingleWriterShards field, defaults to 8 (hardcoded with a "keep in sync" comment, matching how singleWriterEnabled/poolTimeout already avoid the file↔config import cycle).
• main.go — wires file.SetSingleWriterShards(cfg.SingleWriterShards) alongside the existing setters.
• pkg/registry/file/singlewriter_test.go:
  • TestSingleWriter_PriorityOrdering now pins shards=1 for that specific test (via a new setSingleWriterShards helper) — its assertion is inherently about cross-key ordering on ONE committer, which sharding intentionally no longer guarantees; forcing 1 shard preserves the test's original intent.
  • New TestSingleWriter_ConcurrentCommitsDifferentKeys_ShardsRunInParallel: proves actual commit-phase parallelism across shards (peak concurrent commits > 1, and wall-clock well under the fully-serial bound). Verified this fails against the pre-fix single-goroutine behavior (peak=1) and passes with the fix.
  • New TestShardFor_Deterministic: same key always routes to the same shard; a reasonable key set spreads across more than one shard.
• pkg/config/config_test.go — asserts the new default.
• docs/features/single-writer-priority-queue.md — updated for the sharded design; also corrected two stale lines (title/summary still said "prototype, gated off" / "default false", both stale since feat(storage): enable single writer by default and harden pool timeout #393).

Test plan

• go build ./..., go vet clean on touched packages (pre-existing, unrelated unkeyed fields vet noise in pkg/registry/file/callstack confirmed present on unmodified main too)
• go test ./pkg/registry/file/ -race -run 'TestSingleWriter|TestShardFor' -v — all pass, including the new tests
• go test ./pkg/registry/file/... -race -run TestSingleWriter -count=5 — clean, no flakiness
• go test ./... -race — green except the pre-existing TestFileSystemStorageWatchReturnsDistinctWatchers race, independently confirmed to reproduce identically on unmodified main (unrelated to this change — it's in the Watch() path, not the write path)

Context

Diagnosed via kubescape/node-agent's component-tests CI (dozens of ContainerProfile writes/reads across concurrently-starting pods), by directly tracing this code at the deployed version (v0.0.331) and confirming with a live before/after comparison. Also independently ruled out an alternative (CUSTOM_REST_ENABLED, #387) empirically — it shares the same StorageImpl/single-writer instance as the default REST path, so it doesn't touch this bottleneck; a CI run confirmed no improvement.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LLN3CgGftcnAikV13qD6yJ

AI-skills: none | cmds: /oh-my-claudecode:autopilot

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: d0d6bae4-7d43-4830-8cda-9ee044d1a087

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Summary:

• License scan: failure
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: success

[matthyx]

Empirical validation against kubescape/node-agent's real CI

Built a test image from this branch and validated end-to-end against node-agent's actual component-tests suite (not just this repo's own unit tests). Two runs:

Default (8 shards, pool size 10) — node-agent#949:

• 15/30 test jobs failed, down from the 18-19/30 baseline.
• The catastrophic symptom is completely gone: no more sustained 20-minute hangs. A burst of 16 concurrent container-profile creates at test startup all succeeded within 5-9 seconds (previously these same profiles retried for the full test timeout).
• Failure count dropped from 323 failed create attempts (pre-fix) to 6, out of 61 total attempts in one representative test run.
• Residual failures are fast (self-heal in ~1-2 min) and land on different, unrelated assertions per test — a qualitatively different, much less severe failure mode than the original regression.

Higher headroom (16 shards, pool size 24) — node-agent#950:

• 21/32 failed — no better than 8 shards, arguably slightly worse.
• This rules out "just needs more shards" as the explanation for the residual flakiness. Since more concurrency didn't help (and the storage pod's CPU limit was unchanged at 500m across both experiments), the leading hypothesis is that residual failures are capped by the storage pod's CPU allocation under concurrent load, not by shard/pool count — i.e. a separate, follow-up concern from what this PR addresses.

Conclusion: this PR fixes the actual regression (the single-writer-by-default throughput collapse). A smaller, separate flakiness remains in node-agent's CI that doesn't respond to more storage-side concurrency and likely needs its own investigation (CPU limits, or possibly unrelated CI pod churn observed during validation) — tracked independently, not blocking this fix.

🤖 Generated with Claude Code

[github-actions]

Summary:

• License scan: failure
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: success