Claude/determined maxwell rswptp - #1356
Conversation
record_metric opens a connection, INSERTs one row and commits, per metric. The background monitor emits 7 metrics every 30s and the /performance/report endpoint replays a whole client batch through the same path, so a browser sending 50 samples cost 50 connect+commit cycles. Add record_metrics()/_store_metrics(), which take the lock once, extend the buffer and fast-access deques in one critical section, and persist the whole batch with a single connect -> executemany -> commit. record_metric and _store_metric are unchanged for the ~15 genuine single-metric callers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Three tests failed only under the full suite with PYTHONPATH=src. Root cause: the tests patched `perf_mod`, a module-level alias that can bind to a *different* module object than the one PerformanceMonitor's methods read their globals from. This file's preamble re-imports the module, and CI's PYTHONPATH=src lets the package resolve under a second name, so `monkeypatch.setattr(perf_mod, "psutil", fake)` silently no-ops. The real psutil then ran, and on a loaded CI machine cpu/memory exceeded the 80% warning thresholds, firing an alert. `_store_alert` opens its own sqlite3 connection, so the "one cycle, one connection" assertion saw 2, and the process-metrics test saw all 7 samples instead of 4. Resolve the target as sys.modules[PerformanceMonitor.__module__] so the patch lands regardless of import identity, and clear the metrics buffer before each cycle so leftover samples cannot bleed across tests. Separately, the report endpoint now batches through `record_metrics`; its error-path test still patched the singular `record_metric`, which is inert and let the request succeed with 200 instead of 500. Repointed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Review follow-up. The happy-path report test patched the singular `record_metric`, but the endpoint calls `record_metrics`. The patch was therefore inert: the test still returned 200 and still asserted the count, while the request performed a live in-process SQLite write. It passed for the wrong reason and had silently lost its isolation. Repoint the patch to `record_metrics` and assert the batched call shape -- one await for the whole report, carrying both samples with the expected names, values and component. The await-count assertion is the substantive part. It pins the contract this endpoint exists to provide: a regression to one write per metric would preserve both the 200 and `metrics_recorded`, so nothing else in the suite would notice. Prove-failed by reverting the patch target to `record_metric`: the request still returns 200 and the new assertion fails `assert 0 == 1`, confirming the guard catches exactly the defect it was written for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
record_metrics computed a single datetime.now() for the whole batch, so every row shared one identical timestamp. The serial record_metric path stamps each metric at construction time, so batching silently diverged from the promised serial semantics and erased per-sample ordering for callers that submit genuinely distinct samples. Move the now() call into the per-record comprehension so the batch path matches record_metric exactly. Addresses Copilot reviewer feedback on PR #1341. Verified: 128/128 in tests/unit/test_performance_monitor.py, including the serial-vs-batched parity test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DduoiyZdhb27Qb7wWoEq45
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
🔍 PR Validation |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Optimizes performance metric ingestion by batching SQLite writes.
Changes:
- Adds batched metric recording and persistence.
- Batches system-resource and frontend performance reports.
- Adds regression tests for batching behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
performance_monitor.py |
Implements batched metric storage and resource sampling. |
router.py |
Batches frontend performance reports. |
test_performance_monitor.py |
Tests batch persistence and monitoring behavior. |
test_v1_router_extended.py |
Verifies batched report ingestion. |
| await self.record_metrics(samples) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error monitoring system resources: {e}") |
Independent review — head
|
Oldest-first scan of all 65 open PRs against the PR Remediation & Publish Runbook. Same terminal conclusion as the 2026-07-31 run: no autonomous merge path exists; remaining work is human-only (PUBLISH GATE is human by default, auto_merge_policy unset). Material change since last run: 4 non-draft PRs (#1311, #1358, #1359, #1366) are now green and mergeable, awaiting only a human ready/merge decision. Also corrects the 2026-07-31 run's "gate faulting" framing of the agent-completion/truth-gate invalid_payload result: the collected verdicts (#1370, #1356) show it is a legitimate fail-closed verdict for a missing agent-completion contract (no linked canonical issue + no trusted-publisher provenance manifest), not a workflow bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019hqExzDLDCCbRLdofRxcn1
Superseded — the delta this PR carries is now on a clean branchFollowing up on the review above, which found that this PR's only content not already on Rather than leave those staged, that fix has been re-cut off current
Full unit suite on #1400's head: 8105 passed, 5 xpassed, 0 failed. Recommendation: close this PR as superseded once #1400 merges. Not closing it myself — that is a maintainer call, and #1400 has not landed yet. If you would rather land this branch instead, #1400 can be closed and the blockers above still apply here. Generated by Claude Code |
Blocked on two things, both needing the branch ownerPicked up in a scheduled PR-remediation pass. This PR is not a draft, so it is in scope for remediation, but neither blocker can be cleared from outside its branch. 1. Merge conflict. 2. The PR body is the unfilled template.
The truth-gate failure has a second, structural cause worth knowing about. No changes pushed from here; this session is scoped to a different designated branch. Recording terminal state for this pass: HALTED — merge conflict plus incomplete canonical-issue evidence, both requiring the branch owner. Generated by Claude Code |
|
Worked this PR's merge conflict to a resolution, and the resolution turns out to be that there is nothing left to merge. Recording the evidence rather than pushing an empty merge. The conflict. The branch side is correct — a shared What survives that resolution. After merging Six lines. The batching work itself ( And The only production-code difference is comment wording. The 90 test lines are the real gap: #1400 carries regression tests for the per-record timestamp and this branch does not — its test changes were part of the batching commits that already landed. So merging this PR would deliver a fix that #1400 delivers with proof, at the cost of a four-commit merge whose conflict has to be hand-resolved first. Terminal state: Generated by Claude Code |
|
This PR is fully superseded by
The conflict in The only remaining delta is timestamp stamping. now = datetime.now(timezone.utc)
... timestamp=entry.get("timestamp") or now,this branch stamps each record individually. That difference is already carried by #1400 ( Also worth noting independent of the above: the PR body is still the unfilled template ( Terminal state: Generated by Claude Code |
|
Superseded — recommend closingChecked this against
A merge probe against
That single remaining delta is what #1400 implements, on top of the already-merged batching, with strictly better coverage — a walking-clock fixture that pins the Nth record carries the Nth instant rather than asserting timestamps merely differ (wall-clock resolution is too coarse for that to be non-flaky), plus a case proving an explicitly supplied So resolving this conflict would either re-land code already on Two things worth carrying forward
Generated by Claude Code |
`record_metrics` docstring promises "The observable behaviour is identical to calling `record_metric` once per entry". It was not: the batch hoisted a single `now = datetime.now(timezone.utc)` out of the comprehension and gave every record in the batch that same timestamp, while the serial `record_metric` stamps each metric at the moment it is recorded. Nothing tested the timestamp in either direction, so the divergence was free to persist. It was raised on #1341 (Copilot) and again on #1356, and has now survived two reviews unfixed. Consult the clock per record so the batch is a true drop-in. The `entry["timestamp"]` escape hatch is unchanged: an explicitly supplied timestamp is still honoured and the clock is only read for entries that omit it. Cost is one extra clock read per metric — the background monitor records 7 per 30s cycle, so it is not measurable against the SQLite commit the batch exists to collapse. Three tests pin it. Rather than assert timestamps merely differ — wall-clock resolution is coarse enough that several `now()` calls in a tight loop can legitimately return the same value — they patch the module clock to walk a known sequence, so the Nth record must carry the Nth instant. That holds only if the clock is consulted once per record, in order. Two of the three fail against the shared-`now` code and pass with this change; the third guards the explicit-timestamp path, which was never broken. Claude-Session: https://claude.ai/code/session_019baCDT5aP5Z66pLGBCE2Y6 Co-authored-by: Claude <noreply@anthropic.com>
Superseded by #1400 — and merging this now would regress
|
| File | Lines it would add to main |
Lines it would remove from main |
|---|---|---|
services/performance_monitor.py |
3 | 4 |
api/v1/router.py |
11 | 127 |
tests/unit/test_performance_monitor.py |
0 | 73 |
tests/unit/test_v1_router_extended.py |
0 | 665 |
The fix itself is already on main, byte-identical
main:307 timestamp=entry.get("timestamp") or datetime.now(timezone.utc),
#1356:306 timestamp=entry.get("timestamp") or datetime.now(timezone.utc),
The only remaining difference in that file is comment wording — main's phrasing is the later, fuller one. No functional delta.
The 11 router lines are a revert, not a contribution
All eleven are pre-offload synchronous calls:
+ total = data_service.count_videos()
+ paginated_videos = data_service.get_videos_summary(limit=limit, offset=offset)
+ video_detail = data_service.get_video_detail(video_id)
+ learning_log = data_service.get_learning_log()
+ for chunk in transcript_chunks:
+ chunk_events = await _extract_chunk(chunk)
On current main every one of these is already off the event loop — asyncio.to_thread at router.py:953, :1056, :1091, and the chunk loop is asyncio.gather(*(_extract_chunk(chunk) for chunk in window)) at :2067. Merging this branch would reintroduce blocking SQLite I/O into async handlers and serialize chunk extraction, undoing the #1327 / #1392 work.
The tests are a strict subset
Zero added lines against main in both test files, 738 removed. This branch contributes no coverage that main does not already have — and #1400 added the timestamp tests this branch never had.
Recommendation
Close as superseded, pointing at #1400. Nothing here needs salvaging: the fix landed, the tests landed better, and the remainder is stale. Per MERGE_POLICY.md gate 6 the winner has now landed, so the reconciliation is unblocked.
Flagging rather than closing — that call is yours. Recorded as DEFERRED(superseded_by_1400).
Generated by Claude Code
Supersession confirmed — #1400 merged#1400 landed on timestamp=entry.get("timestamp") or datetime.now(timezone.utc),That was this PR's only content not already on The blockers noted earlier still apply and are now moot: the two-hunk conflict was in exactly those timestamp lines, and the red truth gate ( Safe to close as superseded. Leaving the close to you rather than doing it unilaterally. Generated by Claude Code |
|
Please add a See governance: #898 |
Canonical issue
Closes #
Outcome
Describe the user or operational result this PR produces.
Scope
Risk
Verification
List exact automated and manual checks, tied to the current head SHA.
Production evidence
Provide the Vercel preview, production deployment, runtime evidence, or state why production evidence is not applicable.
Agent handoff
Agent provenance
Human-authored pull requests may delete this section. Agent-authored pull requests must replace agent-lock-example with agent-lock-manifest and fill the values. Scope and test paths remain authoritative in the linked issue.
The declared agent publishes a result comment on the linked issue or PR with the exact run ID and current 40-character head SHA. Replace
agent-lock-event-examplewithagent-lock-eventonly when publishing real evidence.