feat: Plan 8 PR B — decision WAL + flush + metrics - #20
Conversation
Add bounded local decision WAL (drop-oldest / no-drop), flush worker with stub transport, proxy decision recording, and sync reconnect metrics. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe edge verifier now persists request decisions in a bounded WAL, flushes decision batches, records synchronization metrics, and wires these components into startup and shutdown. Proxy decisions include trust details, while SSE reconnects receive categorized metrics. ChangesDecision WAL and edge verifier observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EdgeVerifierProxy
participant DecisionWAL
participant FlushWorker
participant FlushTransport
Client->>EdgeVerifierProxy: Send request
EdgeVerifierProxy->>DecisionWAL: Append decision
FlushWorker->>DecisionWAL: Read pending decisions
FlushWorker->>FlushTransport: Flush decision batch
FlushTransport-->>FlushWorker: Return result
FlushWorker->>DecisionWAL: Acknowledge delivered decisions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoAdd bounded decision WAL, stub flush worker, and edge sync/WAL metrics
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Review: PR #20 — Plan 8 PR B: decision WAL + flush + metrics
Well-structured PR. 1032 additions implementing the bounded decision WAL, flush worker with stub transport, and edge metrics. The modularization of main.go into parseFlags/bootstrapDemoIdentity/mustOpenWAL/mustStartSync/startMockBackend is a big improvement.
What is done well
| Area | Notes |
|---|---|
| WAL design | Bounded, append-only, drop-oldest vs no-drop, monotonic wal_seq, disk round-trip with schema versioning |
| Crash-safe disk | tmp → fsync → rename → dir sync (consistent with snapshot persistence) |
| Flush batching | FlushBatch with BatchID (UUIDv4), seq range, and sha256 payload hash → idempotent retries |
| Proxy decision recording | Records allow/deny/passthrough on all paths (invalid-sig, replay, denied, unsigned) |
| Metrics | Reconnect counters by reason, WAL bytes, decisions dropped, snapshot HW, SSE age |
| No-drop sizing | SizedNoDropWALMaxBytes (8GiB floor, 1.5× outage buffer) is well thought out |
| Tests | 4 focused tests covering drop-oldest, no-drop, flush ack + disk round-trip, sizing |
Issues
1. persistLocked() on EVERY Append — performance (High)
w.entries = append(w.entries, walEntry{rec: d, size: size})
w.bytes += size
w.publishBytesLocked()
return w.persistLocked() // full JSON serialize + fsync + rename + dir syncEvery single decision triggers a full WAL serialize to disk, f.Sync(), rename, and dir sync. At even 100 req/s through the proxy, this is 100 full-file fsyncs/sec — likely the hottest syscall path in the whole binary. The crash-safety guarantee of per-record fsync is usually unnecessary for a decision log (decisions are re-collectable; only durability of the ack matters for correctness).
Fix: Persist on a cadence (e.g., every N records or T seconds via the flush worker), or use an append-only JSONL file with periodic fsync. Keep the atomic tmp→rename pattern for the final coalesced write.
2. FlushOnce has no context deadline (Medium)
case <-ticker.C:
_ = w.FlushOnce(ctx)If a future real transport hangs, FlushOnce blocks and the worker can never observe ctx.Done() — graceful shutdown hangs until the process watchdog kills it. The final-flush path correctly uses a 5s timeout; the periodic path should too. A context.WithTimeout inside FlushOnce (or in Run) would close this.
3. reconnectReason relies on string matching (Minor)
case strings.Contains(msg, "recovered from 429"):
return "backlog"Fragile. Prefer sentinel errors (e.g., errBacklog, errGone, errShutdown) wrapped in the returned error so errors.Is works without string scanning. errors is already imported in sse.go.
4. dropOldestLocked re-slicing retains backing array (Minor)
w.entries = w.entries[1:]Repeated appends/drops can keep a growing backing array alive. Not a correctness issue; a ring buffer would bound memory more predictably. Fine to defer.
Minor observations
recordDecisioncallswal.SetNoDrop(active.NoDropDecisions)on every append — takes the WAL mutex each time; could set once at policy change.- The single-record-larger-than-maxBytes acceptance is documented and intentional — good.
StubTransportcorrectly returns immediately, so the current worker never hangs in practice — issue #2 is a "when real transport lands" concern.
Verdict
Approve with changes requested for #1 (persist-on-every-append is a genuine bottleneck). #2 and #3 are recommended before the real flush transport lands. Otherwise this is a clean, well-tested PR.
Keep VERILINK_NO_DROP_DECISIONS sticky against policy, and switch the on-disk WAL to JSONL append + periodic compact instead of O(n) rewrite. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed Qodo actionable findings in
On the RFC 9421 outbound upstream signing rule: acknowledged / out of Plan 8 PR B scope — this edge verifies inbound client signatures only; re-signing outbound upstream hops remains a separate product decision (same as Plan 8 PR A). @coderabbitai review |
|
I will treat outbound upstream signing as out of scope for this PR. If needed, I can help create a follow-up issue for that product decision. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/edgeverifier/proxy.go (1)
204-237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep WAL trust fields from one evaluation.
Store.Loadcan return differentSnapshotgenerations forAllowByScoreand the followingLookupScore. ReturnBlacklistedfrom the same evaluation, or evaluate one loaded snapshot for all fields.- In the
trustStorefallback, assign the fetchedTrustScoreto the namedscoreresult before returning.PrincipalIDremains empty becauseTrustStoreexposes no principal ID.🤖 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 `@internal/edgeverifier/proxy.go` around lines 204 - 237, Update annotateTrust so AllowByScore and LookupScore use the same loaded Snapshot evaluation, deriving blacklisted and score fields from one generation rather than separate Store.Load results. In the trustStore fallback, assign the fetched TrustScore to the named score return value before returning; keep PrincipalID empty.
🤖 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 `@cmd/edge-verifier/main.go`:
- Around line 39-66: Track the flush worker, sync runner, and shutdown goroutine
with a sync.WaitGroup, adding the required sync import. Update the goroutine
launches around NewFlushWorker, syncRunner, and the signal-driven shutdown
routine to call Add before starting and Done on exit, then wait on the group
before main returns so server shutdown and background cleanup complete.
In `@internal/edgeverifier/flush.go`:
- Around line 20-26: Assess the WAL retention configuration around FlushBatch
and the on-disk decision records, then add an age-based retention bound
alongside the existing maxBytes limit so records cannot remain indefinitely
during prolonged flush outages. Enforce the age limit when retaining or reading
WAL entries, expose the setting through the existing configuration path, and
document the expected retention window for the edge WAL.
- Around line 103-107: Update the cancellation branch in Run to repeatedly call
FlushOnce with the existing timeout context until it returns nil with no pending
work, or until a flush error/context deadline stops progress. Preserve
cancellation independence via context.WithoutCancel, ensure cancel is always
called, and retain the existing return after the drain attempt.
- Line 99: Update Run where it creates the ticker from w.interval to use the
default flush interval whenever w.interval is non-positive, while preserving
configured positive intervals. Ensure time.NewTicker always receives a positive
duration, including when FlushWorker is constructed via a composite literal.
- Around line 165-169: Update FlushWorker.logf so the underlying Logger.Printf
receives a constant format string, passing the edgesync prefix and formatted
message as arguments rather than concatenating format with the prefix. Add the
recognized “logf is a printf wrapper” documentation form so go vet continues
validating logf call sites and preserves formatting of args.
- Around line 123-131: Update buildFlushBatch and its BatchID generation so the
identifier is deterministically derived from the batch content instead of using
a fresh random value from newBatchID. Preserve the existing UUID version and
variant formatting when deriving the value, remove the now-unnecessary
randomness/error path, and ensure retries of identical decisions produce the
same BatchID.
In `@internal/edgeverifier/sse.go`:
- Around line 429-449: The reconnectReason function must stop classifying
control signals by substring matching err.Error(), which allows untrusted
upstream response bodies to spoof reasons. Introduce typed or sentinel errors
for shutdown, recovered 429/410, and unavailable signals at their creation
sites, then update reconnectReason to use errors.Is or errors.As while
preserving cancellation/deadline and generic error handling.
In `@internal/edgeverifier/wal_test.go`:
- Around line 204-206: Extend the WAL tests around flushFunc with coverage for
transport failure: perform a failing FlushOnce, assert wal.Len() is unchanged,
then perform a successful flush and verify the original sequences are delivered.
Add a persistence test using a configured Path and small MaxBytes that triggers
dropOldestLocked, reopen the WAL, and assert the dropped decisions are not
resurrected.
- Around line 155-186: The WAL test’s wall-clock comparison in the append
performance test is flaky under real filesystem and scheduler variability.
Replace the small/large timing assertion with a deterministic check of the
algorithmic behavior of Append, such as instrumenting or measuring entries slice
reallocations/copies as WAL length grows, or move the timing logic into a
non-gating benchmark. Preserve coverage that Append remains O(1) with increasing
WAL length and remove the fragile time-based failure from the test.
- Around line 110-118: Update the reload assertion in the wal3 Pending check to
require the exact expected WalSeq value from the prior stream state, rather than
merely requiring it to be positive. Keep the existing fingerprint and
pending-count validations unchanged, and ensure the assertion verifies the
sequence continues past all previously used sequences, including acknowledged
entries.
- Around line 193-198: Update the SizedNoDropWALMaxBytes test in wal_test.go to
assert directly that the 1<<20 and 900 case equals defaultNoDropWALBytes,
removing the redundant outer condition. Add test cases covering negative
p99BytesPerSec and non-positive outageSeconds, verifying both return the
defaultNoDropWALBytes floor.
In `@internal/edgeverifier/wal.go`:
- Around line 396-408: Update loadStream after rebuilding w.entries and w.bytes
to enforce w.maxBytes before returning: trim the oldest loaded entries until the
total is within the configured limit, or return an explicit startup error naming
both the configured limit and loaded size. Preserve the existing staleBytes
reset and successful return only after capacity enforcement.
- Line 189: Update Append and the writeLineLocked flow to build the walLine
once, marshal that representation once, and use the resulting encoded length for
the entry size instead of calling estimateDecisionBytes. Pass or reuse the
marshaled bytes when writing the line so each decision is encoded only once and
w.bytes matches the on-disk representation.
- Around line 551-555: Update the WAL sizing calculation to use math.Ceil
instead of the manual +0.999999 adjustment. Before converting the ceiling result
to int64, compare it with math.MaxInt64 and clamp values above that limit;
preserve the existing defaultNoDropWALBytes minimum handling.
- Around line 339-341: Update loadStream to scan the WAL twice: first determine
the maximum Through value across dec records, then initialize pending and order
and perform a second scan that retains only entries whose WalSeq exceeds
ackThrough. Apply the filtering during the second pass so acknowledged decisions
are never stored, while preserving existing decision loading and ordering
behavior for live entries.
- Around line 282-283: Update dropOldestLocked so the ack watermark is written
without forceSync, allowing walSyncEveryN to batch durability instead of
fsyncing each dropped entry. Also coalesce multiple drops handled by a single
Append into one ack line, avoiding one WAL record per dropped entry and reducing
staleBytes and compaction pressure.
- Around line 411-425: Update DecisionWAL.writeLineLocked to reject any
JSON-encoded walLine whose complete line length, including the newline, exceeds
walMaxLineBytes before writing. Also enforce appropriate bounds on
request-derived fields in Append so oversized records are rejected there without
surfacing as proxy errors for otherwise valid requests.
- Around line 190-200: In the WAL append capacity logic, consolidate the
duplicated no-drop checks around the visible loop using w.noDrop and
dropOldestLocked: perform one pre-check that returns ErrWALFull when the write
exceeds w.maxBytes, then make the dropping loop unconditional for the remaining
case. Preserve the existing w.nextSeq rollback and behavior when entries are
exhausted.
- Around line 262-264: Update AckThrough around writeLineLocked and
maybeCompactLocked to propagate their errors instead of discarding them, while
preserving the existing acknowledgement and compaction flow. Adjust callers to
handle the returned error, including logging the result in
FlushWorker.FlushOnce.
- Around line 497-513: Update compactLocked to install a deferred recovery step
immediately after closing the live WAL handle and clearing w.file. If os.Rename,
syncDir, or os.OpenFile fails, attempt to reopen w.path with the same
create/read-write/append flags and permissions before returning the original
error; ensure successful compaction keeps the newly opened handle and resets the
existing counters.
- Around line 440-447: Update the compaction decision before compactLocked so it
requires both a minimum absolute staleBytes threshold and staleBytes dominating
the live bytes by the intended ratio; remove the inverted large-WAL condition
that allows small stale ratios to compact. Preserve the drain-to-empty behavior
when bytes is zero and keep compactLocked as the compaction path.
- Around line 344-352: Update loadStream’s JSONL scanning loop to tolerate a
json.Unmarshal failure only when the failed raw line is the final line, treating
it as a torn WAL tail and stopping recovery; retain the existing error for
malformed lines with subsequent content. Restructure the scanner using one-line
lookahead or equivalent buffering so the next-line check does not skip data.
---
Outside diff comments:
In `@internal/edgeverifier/proxy.go`:
- Around line 204-237: Update annotateTrust so AllowByScore and LookupScore use
the same loaded Snapshot evaluation, deriving blacklisted and score fields from
one generation rather than separate Store.Load results. In the trustStore
fallback, assign the fetched TrustScore to the named score return value before
returning; keep PrincipalID empty.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ab4318e-d8db-4808-b4ce-e2f395f77fac
📒 Files selected for processing (8)
cmd/edge-verifier/main.godocs/superpowers/plans/HANDOVER.mdinternal/edgeverifier/flush.gointernal/edgeverifier/metrics.gointernal/edgeverifier/proxy.gointernal/edgeverifier/sse.gointernal/edgeverifier/wal.gointernal/edgeverifier/wal_test.go
Single-marshal appends, torn-tail recovery, surfaced ack/drop write errors, typed SSE reconnect reasons, AllowByScore blacklist reuse, deterministic batch IDs, flush-all on shutdown, and WaitGroup join. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the requested resilience/perf items in this push, plus overlapping CodeRabbit majors:
Also: deterministic batch IDs, flush-all on shutdown, WaitGroup before exit, compact reopen-on-failure, maxBytes trim after reload, line-size cap at write, removed flaky wall-clock test, @coderabbitai review |
|
✅ Action performedReview finished.
|
Apply tenant no-drop via SyncRunner on snapshot/policy updates instead of per-request SetNoDrop. Keep the live WAL fd until compacted file is reopened, bound periodic flush with a timeout, and return trustStore score from annotateTrust. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the additional review notes (plus remaining open CR items triage): Actionable (this push)
Reviewed / no code change
Still deferred (open CR threads)
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== Go diff stat ==='
git diff --stat HEAD^ HEAD -- '*.go'
printf '%s\n' '=== Source outlines ==='
for f in internal/edgeverifier/wal.go internal/edgeverifier/flush.go internal/edgeverifier/sse.go internal/edgeverifier/proxy.go cmd/edge-verifier/main.go internal/edgeverifier/wal_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
printf '%s\n' '=== Focused implementation matches ==='
rg -n -C 8 \
'func \(w \*DecisionWAL\) (Append|AckThrough|compact|Close|SetNoDrop)|func \(.*FlushWorker.*\) (Run|FlushOnce)|WithDecisionWAL|SetNoDrop|annotateTrust|func reconnectReason|context\.WithTimeout|WaitGroup|FlushOnce' \
internal/edgeverifier cmd/edge-verifier 2>/dev/null || true
printf '%s\n' '=== WAL and flush test references ==='
rg -n -C 5 \
'TestWALCompactKeepsAppendWorking|FlushOnce|timeout|NoDrop|AckThrough|compact|shutdown|recovery' \
internal/edgeverifier/*_test.go 2>/dev/null || trueLength of output: 49216 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Current commit diff ==='
git diff --unified=80 HEAD^ HEAD -- \
cmd/edge-verifier/main.go \
internal/edgeverifier/flush.go \
internal/edgeverifier/proxy.go \
internal/edgeverifier/sse.go \
internal/edgeverifier/wal.go \
internal/edgeverifier/wal_test.go
printf '%s\n' '=== Main startup and shutdown ==='
sed -n '35,125p' cmd/edge-verifier/main.go
sed -n '185,225p' cmd/edge-verifier/main.go
printf '%s\n' '=== WAL append, acknowledgement, and compaction ==='
sed -n '86,325p' internal/edgeverifier/wal.go
sed -n '440,620p' internal/edgeverifier/wal.go
printf '%s\n' '=== Flush worker ==='
sed -n '76,185p' internal/edgeverifier/flush.go
printf '%s\n' '=== Sync policy application and event dispatch ==='
sed -n '98,180p' internal/edgeverifier/sse.go
sed -n '292,375p' internal/edgeverifier/sse.go
sed -n '455,490p' internal/edgeverifier/sse.go
printf '%s\n' '=== Proxy trust annotation and decision recording ==='
sed -n '87,270p' internal/edgeverifier/proxy.go
printf '%s\n' '=== Relevant test bodies ==='
sed -n '202,280p' internal/edgeverifier/wal_test.goLength of output: 70666 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Numbered acknowledgement and drop paths ==='
nl -ba internal/edgeverifier/wal.go | sed -n '255,325p'
printf '%s\n' '=== WAL persistence and test hooks ==='
rg -n -C 4 \
'writeBytesLocked|writeLineLocked|os\.OpenFile|chmod|permission|read-only|AckThrough|dropOldestLocked|write error|Sync\(\)' \
internal/edgeverifier/*_test.go internal/edgeverifier/wal.go
printf '%s\n' '=== Full test function map ==='
ast-grep outline internal/edgeverifier --items all --type function --match 'Test.*WAL|Test.*Flush' 2>/dev/null || trueLength of output: 971 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Numbered acknowledgement and drop paths ==='
awk 'NR >= 255 && NR <= 325 { printf "%6d\t%s\n", NR, $0 }' internal/edgeverifier/wal.go
printf '%s\n' '=== Existing WAL failure-path tests ==='
rg -n -C 4 \
'AckThrough|dropOldestLocked|write.*fail|permission|read-only|chmod|Sync\(\)|os\.OpenFile' \
internal/edgeverifier/*_test.go || true
printf '%s\n' '=== WAL test declarations ==='
rg -n '^func Test.*(WAL|Flush)' internal/edgeverifier/*_test.go || trueLength of output: 4744
The no-drop policy synchronization, compaction ordering, periodic flush timeout, shutdown wait, and trust-score assignment match the stated changes. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/edgeverifier/wal.go (1)
208-243: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign reload size accounting with the append-path encoding.
Appendnow sizes each entry from the encodedwalLine(line 218), including the newline.loadStreamsizes reloaded entries withestimateDecisionBytes, which marshals theDecisionstruct instead (line 440). The two encodings differ:walLineuses the short keyt, addsType, and omits empty fields, whileDecisionemitswal_seq,fingerprint,score,blacklisted,action, anddecided_atunconditionally.After a restart,
w.bytestherefore no longer tracks the same unit thatmaxBytesenforcement and the drop loop assume, so the post-reload trim at lines 343-350 and the firstAppenddecisions run on skewed numbers. Reuse one helper that builds thewalLineand returns its encoded length for both paths.♻️ Proposed fix: share one sizing helper
+func decisionLineBytes(d Decision) (int64, error) { + encoded, err := json.Marshal(decisionLine(d)) + if err != nil { + return 0, err + } + return int64(len(encoded) + 1), nil +}Then use
decisionLineBytesinloadStreamin place ofestimateDecisionBytes, and build thewalLineinAppendthrough the samedecisionLineconstructor.🤖 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 `@internal/edgeverifier/wal.go` around lines 208 - 243, Unify WAL entry size accounting by adding a shared decisionLine constructor/helper that builds the walLine and returns its encoded length, including the newline. Update Append to use this helper instead of constructing and sizing the line separately, and update loadStream to call decisionLineBytes rather than estimateDecisionBytes so reload, trimming, and maxBytes enforcement use identical encoding sizes.
🤖 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 `@cmd/edge-verifier/main.go`:
- Line 115: Add the signal shutdown goroutine to the `bg` wait group before
starting it, and ensure the goroutine calls `bg.Done()` on exit. Keep the
existing shutdown sequence, including `server.Shutdown()` and
`backendServer.Shutdown()`, so `bg.Wait()` does not return until signal-driven
shutdown completes.
- Around line 116-118: Update the shutdown cleanup around decisionWAL.Close() to
capture and log any close error, then exit with a non-zero status when
persistence fails. Preserve the nil check and normal successful shutdown
behavior.
In `@internal/edgeverifier/flush.go`:
- Around line 90-92: Add the non-positive interval fallback inside
FlushWorker.Run before the time.NewTicker call, preserving the existing
constructor normalization in NewFlushWorker. Ensure composite-literal workers
use 5 seconds when interval is zero or negative, preventing ticker creation from
panicking.
- Around line 119-135: Update FlushWorker.FlushAll to guard against a nil
receiver, then measure progress using the oldest pending/acked sequence rather
than WAL.Len(), since concurrent DecisionWAL.Append calls can increase the queue
during FlushOnce. Capture the sequence before each FlushOnce and require it to
advance after a successful flush; retain the existing context cancellation,
empty-WAL, and no-progress error behavior.
In `@internal/edgeverifier/sse.go`:
- Around line 104-110: Update SyncRunner.syncWALPolicy so DecisionWAL.SetNoDrop
is always synchronized with the current snapshot: use the active policy’s
NoDropDecisions when present, and explicitly clear it when ActivePolicy returns
nil. Add coverage for transitioning from a policy with NoDropDecisions true to a
snapshot with Policy nil, verifying drop-oldest behavior is restored.
In `@internal/edgeverifier/wal_test.go`:
- Around line 153-184: Add a new test alongside TestWALTornTrailingLineRecovered
that appends a malformed WAL record followed by a valid record, then verifies
NewDecisionWAL returns an error. Reuse the existing WAL setup and cleanup
pattern, and ensure the malformed line is not treated as a recoverable trailing
fragment when another valid line follows it.
- Around line 211-233: Update the compaction test around the payload
construction and AckThrough call to use a printable repeated payload via the
strings package, keeping the encoded line within walMaxLineBytes while matching
the intended size comment. After AckThrough(lastSeq), stat the WAL file and
assert its size reflects compaction before appending the post-compaction
decision, so the test fails if compactLocked does not run.
In `@internal/edgeverifier/wal.go`:
- Line 620: Clamp the value produced by math.Ceil in the WAL sizing calculation
before converting it to int64, ensuring values above math.MaxInt64 use the
maximum representable int64. Update the calculation around calc in the WAL
sizing logic while preserving the existing defaultNoDropWALBytes fallback
behavior for valid in-range results.
- Around line 342-350: Update the reload capacity enforcement around
w.dropOldestLocked so no-drop mode does not silently continue with an oversized
WAL: when w.noDrop is true and w.bytes exceeds w.maxBytes, fail startup with an
explicit error or emit an actionable log containing the configured limit and
loaded size before continuing. Preserve the existing trimming behavior for
drop-enabled mode.
---
Outside diff comments:
In `@internal/edgeverifier/wal.go`:
- Around line 208-243: Unify WAL entry size accounting by adding a shared
decisionLine constructor/helper that builds the walLine and returns its encoded
length, including the newline. Update Append to use this helper instead of
constructing and sizing the line separately, and update loadStream to call
decisionLineBytes rather than estimateDecisionBytes so reload, trimming, and
maxBytes enforcement use identical encoding sizes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b68b56a8-006f-4d4d-a856-68c7064bf402
📒 Files selected for processing (7)
cmd/edge-verifier/main.gointernal/edgeverifier/flush.gointernal/edgeverifier/policy.gointernal/edgeverifier/proxy.gointernal/edgeverifier/sse.gointernal/edgeverifier/wal.gointernal/edgeverifier/wal_test.go
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Re-review: PR #20 (3 new commits: d4b4dd1, 8cd5f86, 46a82df)
Verdict: Approve. All prior findings are addressed, and the WAL rewrite is a genuinely better design.
Prior issues — all resolved
| Prior finding | Resolution |
|---|---|
| persist-on-every-append (fsync per decision) | WAL rewritten to append-only JSONL (meta/dec/ack lines) with batched fsync every 32 lines; compaction only when stale ≥1 MiB and ≥ half of live bytes |
| FlushOnce no context deadline | context.WithTimeout(ctx, 5s) on both periodic and shutdown flush paths |
| reconnectReason string matching | Sentinel errors (errSSEBacklog, errSSEGone, errSSEUnavailable, errSSEShutdown) + errors.Is |
| SetNoDrop on request hot path | Moved to syncWALPolicy() on bootstrap / snapshot recover / policy.replace; SetNoDrop is now sticky-merge with no-op-when-unchanged |
What is done well
- Crash-safety tradeoffs are correct and reasoned: un-fsynced dec/ack lines at worst cause re-delivery, which is safe because BatchID is now a deterministic hash of the payload (
buildFlushBatch) — retries dedupe on identical content. The torn-tail recovery (TestWALTornTrailingLineRecovered) handles partial last-line writes. - fd swap in
compactLockedkeeps the live handle until rename + reopen, so Append never observes a nil file — andTestWALCompactKeepsAppendWorkingverifies it. Dir-sync failure is surfaced as an error, not swallowed. Close()on shutdown withbg.Wait()ordering means the finalFlushAlldrains before the file handle is closed.- Forced (CLI/env) no-drop is sticky and can never be cleared by a synced policy — correct precedence.
AckThroughreturns errors and the flush worker now surfaces ack failures instead of ignoring them.AllowByScorereturningblacklisteddirectly removes the redundantLookupScorecall in proxy.
Minor observations (non-blocking)
dropOldestLockedstill usesw.entries = w.entries[1:](backing-array retention) whileAckThroughuses the proper copy — inconsistent, but bounded and not a correctness issue.- On startup
loadStreammaterializes the entire WAL into a map + order slice; fine for steady-state sizes, worth noting for the 8 GiB no-drop case (boot cost). - Deterministic BatchID is only collision-free while payloads differ;
wal_seqguarantees that in practice.
No further changes required. Ship it — go test and vet both pass locally.
Age-based WAL retention (default 24h), two-pass load for bounded recovery memory with correct nextSeq watermarks, FlushAll ack-seq progress, shutdown WaitGroup + WAL close errors, no-drop oversize startup failure, and related SSE/test hardening. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Closed all remaining open review threads in this push (no deferrals):
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Latest changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== Source outlines ==='
for f in internal/edgeverifier/wal.go internal/edgeverifier/flush.go internal/edgeverifier/sse.go cmd/edge-verifier/main.go internal/edgeverifier/wal_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
printf '%s\n' '=== Focused source matches ==='
rg -n -C 10 \
'MaxAge|load|recover|AckThrough|dropOldestLocked|writeLineLocked|compact|SizedNoDropWALMaxBytes|FlushAll|FlushOnce|func \(.*FlushWorker.*\) Run|syncWALPolicy|SetNoDrop|WaitGroup|signal|Close\(\)' \
internal/edgeverifier cmd/edge-verifier 2>/dev/null || true
printf '%s\n' '=== WAL and flush test declarations and relevant assertions ==='
rg -n -C 8 \
'^func Test.*(WAL|Flush)|MaxAge|corrupt|oversize|compact|AckThrough|dropOldest|FlushAll|no.drop|NoDrop|MaxInt64|Close' \
internal/edgeverifier/*_test.go 2>/dev/null || true
printf '%s\n' '=== Latest focused diff ==='
git diff --unified=35 HEAD^ HEAD -- \
internal/edgeverifier/wal.go \
internal/edgeverifier/flush.go \
internal/edgeverifier/sse.go \
cmd/edge-verifier/main.go \
internal/edgeverifier/wal_test.goLength of output: 216405
Persist the I verified the reported retention, two-pass recovery, no-drop load limit, You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/edgeverifier/wal.go (2)
444-459: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the encoded line length for the reloaded entry size.
Appendnow sizes each entry from the encodedwalLine(size = int64(len(encoded)), line 242).loadStreamsizes reloaded entries withestimateDecisionBytes(d), which marshals theDecisionstruct.Decisionuses the long JSON keys (wal_seq,fingerprint,decided_at) whilewalLineuses short keys withomitempty, so the two values differ for the same record.The consequences after a restart:
w.bytesand theWALBytesmetric no longer track the on-disk size.- The capacity checks in
openAndLoad(lines 388-395) and inAppendcompare a mixed accounting base againstw.maxBytes, so the no-drop startup error at line 390 can fire or stay silent for the wrong loaded size.The raw line is available in the second pass. Use its length, which matches what
Appendrecorded.🛠️ Proposed fix
for sc.Scan() { raw := sc.Bytes() if len(raw) == 0 { continue } + lineSize := int64(len(raw)) + 1 // trailing newline, as written by Append var line walLine- size := estimateDecisionBytes(d) - w.entries = append(w.entries, walEntry{rec: d, size: size}) - w.bytes += size + w.entries = append(w.entries, walEntry{rec: d, size: lineSize}) + w.bytes += lineSize🤖 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 `@internal/edgeverifier/wal.go` around lines 444 - 459, Update loadStream’s second-pass entry sizing to use the raw walLine’s encoded length rather than estimateDecisionBytes(d). Preserve the existing Decision reconstruction and append flow, but set each walEntry size and w.bytes accounting from the available line length so reloads match Append’s on-disk sizing.
399-443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that the watermark pass and the load pass stay consistent.
scanWALWatermarksandloadStreamduplicate the scan loop, the buffer limits, and the torn-tail rule. The two loops must agree, otherwise the first pass can accept a file that the second pass rejects, or the reverse. One shared scan helper that yieldswalLinevalues would remove the duplication.Also confirm the reload cost. The WAL file can reach
maxBytes(8 GiB in no-drop mode), and startup now reads it twice before the edge verifier serves traffic.Also applies to: 460-527
🤖 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 `@internal/edgeverifier/wal.go` around lines 399 - 443, Refactor scanWALWatermarks and loadStream to use one shared WAL scanning helper that owns scanner buffer limits, JSON decoding, and torn-tail handling, yielding walLine values consistently to both passes. Update both callers to preserve their existing watermark and entry-processing behavior, then avoid rereading the full WAL during startup where possible so a maxBytes-sized file is not scanned twice before serving traffic.
🤖 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 `@cmd/edge-verifier/main.go`:
- Around line 184-195: Validate cfg.walMaxAge in mustOpenWAL before constructing
the WAL, failing startup when the duration is negative instead of passing it to
WALConfig.MaxAge. Update envDuration to log a clear parse failure when the
configured value is malformed, while retaining the fallback behavior as
appropriate. Ensure both flag and environment-derived values surface invalid
input rather than silently applying the default.
---
Outside diff comments:
In `@internal/edgeverifier/wal.go`:
- Around line 444-459: Update loadStream’s second-pass entry sizing to use the
raw walLine’s encoded length rather than estimateDecisionBytes(d). Preserve the
existing Decision reconstruction and append flow, but set each walEntry size and
w.bytes accounting from the available line length so reloads match Append’s
on-disk sizing.
- Around line 399-443: Refactor scanWALWatermarks and loadStream to use one
shared WAL scanning helper that owns scanner buffer limits, JSON decoding, and
torn-tail handling, yielding walLine values consistently to both passes. Update
both callers to preserve their existing watermark and entry-processing behavior,
then avoid rereading the full WAL during startup where possible so a
maxBytes-sized file is not scanned twice before serving traffic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cdbb1d2-8151-4e4a-82fa-5645ebb1ecdc
📒 Files selected for processing (5)
cmd/edge-verifier/main.gointernal/edgeverifier/flush.gointernal/edgeverifier/sse.gointernal/edgeverifier/wal.gointernal/edgeverifier/wal_test.go
Fail fast on -wal-max-age < 0 so identifier retention cannot be disabled via flag/env, and log malformed VERILINK_WAL_MAX_AGE parse errors. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the remaining finding: reject negative @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Latest changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== Latest relevant diff ==='
git diff --unified=45 HEAD^ HEAD -- \
cmd/edge-verifier/main.go \
internal/edgeverifier/wal.go \
internal/edgeverifier/wal_test.go \
internal/edgeverifier/flush.go
printf '%s\n' '=== Declaration map ==='
for file in cmd/edge-verifier/main.go internal/edgeverifier/wal.go internal/edgeverifier/wal_test.go; do
if [ -f "$file" ]; then
printf '%s\n' "--- $file"
ast-grep outline "$file" --items all 2>/dev/null || true
fi
done
printf '%s\n' '=== Configuration and persistence paths ==='
rg -n -C 12 \
'wal.max.age|WAL_MAX_AGE|parseDuration|ParseDuration|MaxAge|AckThrough|dropOldestLocked|writeLineLocked|writeBytesLocked' \
cmd/edge-verifier/main.go internal/edgeverifier/wal.go internal/edgeverifier/wal_test.goLength of output: 54909
One WAL durability finding remains.
If Write the I verified this by static inspection. I did not execute tests. You are interacting with an AI system. |
Drop loops write a single ack watermark instead of one line per drop, and reallocate the entry slice when head-slicing leaves excess capacity. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Closed the last soft-deferred polish items:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Latest changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== WAL declaration map ==='
ast-grep outline internal/edgeverifier/wal.go --items all
printf '%s\n' '=== Latest WAL diff ==='
git diff --unified=80 HEAD^ HEAD -- internal/edgeverifier/wal.go internal/edgeverifier/wal_test.go
printf '%s\n' '=== Acknowledgement, retention, and compaction paths ==='
rg -n -C 18 \
'func \(w \*DecisionWAL\) (Append|AckThrough|dropOldestLocked|pruneExpiredLocked|compactEntriesLocked|load|compact)|writeLineLocked|staleBytes|DecisionsDropped|MaxAge' \
internal/edgeverifier/wal.go
printf '%s\n' '=== Relevant test coverage ==='
rg -n -C 8 \
'^func Test.*(WAL|Flush)|drop|Drop|ack|Ack|age|Age|compact|Compact|write.*fail|failure' \
internal/edgeverifier/wal_test.goLength of output: 49765
If Coalescing is valid, but do not mutate the live WAL state until the coalesced Add write-failure injection tests for You are interacting with an AI system. |
1 participant
Summary
wal.go) with drop-oldest +decisions_dropped_total, enterprise no-drop, optional disk persist (VERILINK_WAL_PATH)FlushTransport(stub until CP decision ingest / PR C)sync_reconnects_total{reason}wal_bytes,decisions_dropped_total,snapshot_high_water,sse_bytes_age, reconnect reasonsVERILINK_WAL_PATH,VERILINK_WAL_MAX_BYTES,VERILINK_NO_DROP_DECISIONSTest plan
go test ./internal/edgeverifier/... ./cmd/edge-verifier/...@coderabbitai review
Summary by CodeRabbit
New Features
Documentation