LAB-744: fix Review Panel findings (data race, ZK checkpoint leak, restore boot-loop, publish ordering) - #5
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe ingester now rejects far-future events, synchronises sliding-window access, excludes sentiment from checkpoints, tolerates malformed restores, and centralises cache refresh error handling. Startup checkpoint failures are handled safely, with regression tests and README documentation updated. ChangesIngester resilience
Sequence Diagram(s)sequenceDiagram
participant Publisher
participant WindowStore
participant CacheKit
participant Backend
Publisher->>WindowStore: Probe recomputation
Publisher->>CacheKit: Invalidate cache wrapper
Publisher->>CacheKit: Recompute and write
CacheKit->>Backend: Store refreshed value
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ingester/tests/test_windows.py (1)
134-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFence the global switch interval so it can't leak.
sys.setswitchinterval(1e-9)is process-global, and the main-threadstart.wait()on Line 138 sits outside thefinallythat restores it — anything raising there leaves every later test running at a 1 ns switch interval. A timeout on the barrier also turns a hang into a failure.🧯 Move the guard to cover thread start and the barrier
old_interval = sys.getswitchinterval() sys.setswitchinterval(1e-9) - t = threading.Thread(target=writer) - t.start() - start.wait() try: + t = threading.Thread(target=writer) + t.start() + start.wait(timeout=10) for i in range(2000): now = (20_000_400 + i) * 60.0 store.snapshot(now) store.merged("24h", now) except Exception as exc: errors.append(repr(exc)) finally: t.join(timeout=10) sys.setswitchinterval(old_interval)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ingester/tests/test_windows.py` around lines 134 - 150, Move the try/finally boundary in the test around setting the switch interval, starting the writer thread, and waiting on start, so any exception or barrier timeout still restores the original interval. Keep the snapshot/merged loop and error assertion behavior unchanged, and ensure the writer thread is joined before restoring the interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ingester/src/skyline_ingester/windows.py`:
- Around line 244-275: Update the restore logic around Bucket and
_coerced_counter to reject or clamp negative checkpoint counts, including n and
counter values, so restored totals cannot be reduced. Also enforce an upper
bound on checkpoint minute values relative to the current time, skipping
far-future buckets while preserving valid recent buckets.
- Around line 116-135: Update the memoization flow in merged around _memo writes
and add to maintain a generation counter: increment it whenever add invalidates
_memo, capture the generation before _copy_range, and only store the computed
result if the generation is unchanged. Prevent pre-add results from being
reinserted while preserving existing memo lookup and clearing behavior.
In `@ingester/tests/test_extract.py`:
- Around line 82-86: Update the window-preservation assertion in the test around
ingest_raw so merged("24h", NOW) cannot reuse the cached memoized Bucket;
advance the test time or inspect the underlying _buckets directly, while
retaining the expected healthy count and checkpoint assertions.
---
Nitpick comments:
In `@ingester/tests/test_windows.py`:
- Around line 134-150: Move the try/finally boundary in the test around setting
the switch interval, starting the writer thread, and waiting on start, so any
exception or barrier timeout still restores the original interval. Keep the
snapshot/merged loop and error assertion behavior unchanged, and ensure the
writer thread is joined before restoring the interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7127d79f-10fa-4d1c-93a5-22b7afc59ac3
📒 Files selected for processing (8)
ingester/README.mdingester/src/skyline_ingester/jetstream.pyingester/src/skyline_ingester/publisher.pyingester/src/skyline_ingester/windows.pyingester/tests/test_checkpoint.pyingester/tests/test_extract.pyingester/tests/test_publisher.pyingester/tests/test_windows.py
Applies the surviving Review Panel findings from PR #3. - CRIT concurrency: WindowStore was mutated by consume() on the event-loop thread while the publish/checkpoint loops read it via asyncio.to_thread, unsynchronised ("RuntimeError: dictionary changed size during iteration" under load). Guard all _buckets/_memo access with a threading.Lock. - MAJ zero-knowledge: the unencrypted checkpoint persisted the per-language sentiment totals the @cache.secure cache encrypts, letting the backend reconstruct avg = sum/count. Drop sent from snapshot(); the secure 1h window repopulates within an hour of a restart. - MAJ boot-loop: a malformed/partial checkpoint crashed restore() and, via the one unguarded startup path, propagated through asyncio.run into a permanent boot loop. restore() now skips bad entries; the startup read is guarded. - MAJ publish ordering: publish invalidated the key before recomputing, so a failed recompute left it deleted (a miss on the metered-miss path). Recompute first, invalidate + write only on success. - Cleanup: collapse the field-identical Merged into Bucket; anchor _prune retention floor to the added minute so one bogus far-future timestamp can no longer permanently zero the window. Docs: README secure/checkpoint sections state the zero-knowledge boundary. Tests: one regression test per finding; ruff + 39 tests green.
- NEW-1 CRIT: drop future-dated Jetstream events at the ingest boundary (> 300 s past wall-clock). One untrusted time_us could set a _prune retention floor that wiped the whole 24h window, poisoned the next checkpoint, and — as a resume cursor — skipped every real event after reconnect. Dropped frames advance nothing. - NEW-3 MAJ: merged()/snapshot() no longer hold the store lock across the O(window) merge/serialize. Buckets are copied under the lock in 16-bucket chunks (dict.update C-copy — Counter(mapping) copies via a per-key Python loop) and merged outside it. Copies, not references: hot buckets' Counters mutate in place and iterating one mid-merge is the round-1 race class. Deterministic every-tick stalls (40-150 ms measured by the panel at 5 posts/s) drop to ~0.03 ms median; remaining sporadic ~20 ms worst-case is CPython GC pauses, unrelated to the lock. - NEW-2 MAJ: restore() coerces counter keys/values (str/int) inside its existing try, so a poisoned-but-valid checkpoint (e.g. a string counter value) skips that bucket at restore instead of detonating hours later in merged()/most_common() as silent misses. - MIN: _refresh/README now state the honest ceiling — the probe covers recompute failure only; a backend write failure after invalidate still briefly deletes the key (no atomic set in the decorator API). Dropped the checkpoint's double-snapshot probe (pure in-memory recompute; the probe just doubled the work). merged() documents the lock contract.
- merged() memoises its result only when no add() landed since the merge started (generation counter): add() clears the memo, and re-inserting a pre-add view would serve it stale to every same-second caller. - restore() clamps what coercion alone let through: negative counts (would skew ppm/lang_mix) and future minutes (would park a bucket forever) skip the bucket, consistent with the untrusted-checkpoint posture. - test fix: the 'window not wiped' assertion now forces a fresh merge (NOW+1) instead of re-reading the memoised Bucket, which would have passed even on a wiped store.
27Bslash6
force-pushed
the
lab-744-review-fixes
branch
from
July 27, 2026 10:26
61f2b29 to
b6a5645
Compare
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ingester/tests/test_windows.py`:
- Around line 158-172: Move the try/finally in the test around t.start(),
start.wait(), and the snapshot/merged loop so
sys.setswitchinterval(old_interval) always executes if any setup or test
operation raises; preserve the existing exception collection and thread join
behavior.
- Around line 170-174: Update the thread cleanup in the test around t.join and
the writer thread creation so the thread is marked daemon and completion is
explicitly asserted after the 10-second join. Fail the test when the thread
remains alive, before checking errors, while preserving the existing error
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b32a4bda-d170-491d-bbad-f9768ec1e128
📒 Files selected for processing (8)
ingester/README.mdingester/src/skyline_ingester/jetstream.pyingester/src/skyline_ingester/publisher.pyingester/src/skyline_ingester/windows.pyingester/tests/test_checkpoint.pyingester/tests/test_extract.pyingester/tests/test_publisher.pyingester/tests/test_windows.py
🚧 Files skipped from review as they are similar to previous changes (5)
- ingester/tests/test_publisher.py
- ingester/tests/test_checkpoint.py
- ingester/src/skyline_ingester/jetstream.py
- ingester/src/skyline_ingester/publisher.py
- ingester/src/skyline_ingester/windows.py
added 2 commits
July 27, 2026 22:56
- test_windows race test: move t.start()/start.wait() inside try so the
1ns switch interval is always restored; daemon writer + is_alive assert
so a real deadlock fails the test instead of passing silently (CodeRabbit)
- restore(): log each corrupt checkpoint bucket skipped (truncated repr) —
checkpoint corruption is now visible, with a caplog regression assert (Kody)
- publisher: structured extra={operation, window} on failure logs (Kody)
- Rejected in-thread: base-image rule misfire on Python imports (no
Dockerfile in repo); _refresh's broad except is the deliberate per-item
isolation boundary of the publish loop (documented in its docstring)
Round 3 (bug-hunter / security / craftsman / catchphrase) on the PR head.
Closure scorecard: all four round-2 findings CLOSED (NEW-1 skew guard,
NEW-2 coercion, NEW-3 chunked lock — closure re-measured: add() stall
p99 ~0ms vs 40-150ms before — and the doc ceiling). Two MAJ survivors,
both fixed here:
- ZK integrity [MAJ]: restore() read legacy `sent` from the plaintext
checkpoint, so a backend operator (the ZK adversary, who can poison the
checkpoint) could choose the plaintext the next @cache.secure publish
encrypts — proven with injected avg=999/inf. restore() now ignores
`sent` entirely; sentiment repopulates from live ingestion only.
Reverses round-2's 'harmless legacy bridge' on new evidence; also
closes the NaN/inf-via-float(s) hole bug-hunter reproduced.
- Boot loop reopened [CRIT-class]: {'n': inf} in a poisoned checkpoint
raised OverflowError (not in restore()'s except tuple) and
store.restore() ran outside restore_checkpoint's try — reproduced
startup crash that would loop for the checkpoint's 26h TTL. Both
guards added; regression tests for each.
- Observability: warn on whole-checkpoint rejection (version/shape) and
on frames without int time_us — same silent-degradation class round 2
flagged.
- Honesty/cuts: module docstring no longer overclaims the memo (it is
suppressed by concurrent adds); _fast_counter_copy deleted — measured
parity with Counter.copy() (2.18 vs 2.19 us/500-key copy), the
justifying stdlib claim was false; _copy_range sentinel bounds ->
defaults.
Rejected (reasons on the PR): probe/wrapper dedup re-litigates round-2's
veto on dual callables; restore cardinality caps are the vetoed broad-
validation class — the try-wrap closes the boot-loop consequence.
44 tests green, ruff clean.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Contributor
Author
|
@coderabbitai review |
Contributor
Author
|
@kody start-review |
✅ Action performedReview finished.
|
Contributor
Author
|
@27Bslash6 — ready for your signoff / merge. All automated gates are green on head A formal review-request could not be used here: you authored this PR, so GitHub silently drops a request naming you. This mention, plus the |
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.
Follow-up to the merged PR #3, applying the surviving findings from the expert-panel review on this Stage-2 py ingester. Closes LAB-744.
Findings fixed
CRIT — WindowStore data race.
consume()mutates the store on the event-loop thread while the publish/checkpoint loops read it viaasyncio.to_thread; unsynchronised, iterating_bucketsduring a concurrentadd()raisedRuntimeError: dictionary changed size during iterationunder load (masked by a try/except into silent cache misses on the metered-miss product). Now guarded by athreading.Lockacross every_buckets/_memoaccess.MAJ — zero-knowledge checkpoint leak. The unencrypted checkpoint persisted the per-language sentiment totals that the
@cache.securecache encrypts, so the backend could reconstruct the protected value (avg = sum / count).snapshot()no longer writessent; the secure 1h window repopulates within an hour of a restart and the restart-critical counts are unaffected.MAJ — restore boot-loop. A malformed or partial checkpoint crashed
restore()and, via the one unguarded startup path, propagated throughasyncio.runinto a permanent boot loop.restore()now validates and skips bad entries, and the startup read is guarded.MAJ — publish ordering. Publish invalidated the key before recomputing, so a failed recompute left it deleted until the next tick (a miss on the metered-miss path). It now recomputes first and invalidates + writes only on success (publish, secure, and checkpoint paths).
Cleanup. Collapsed the field-identical
MergedintoBucket; anchored_pruneto the added minute so a single bogus far-future timestamp can no longer permanently zero the window (it now self-heals).Verification
ruff check+ruff format --checkclean.Review gate
The crypto/zero-knowledge gate is satisfied by the expert panel that identified these findings; the ZK fix is a strict reduction of persisted plaintext and cannot introduce a new crypto path. No SDK internals touched.
Ticket
Closes LAB-952 (review-debt closure; panel rounds originated on LAB-744).