Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Pause and resume logs can incorrectly report live-feed state for background or canceled offers.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds ordered backlog edge logging so connector operators can observe warnings, pauses, resumes, and recoveries.
Changes:
- Logs queue transitions with depths and thresholds.
- Preserves callback ordering and panic containment.
- Adds integration and callback-panic coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/connector/queue.go |
Reports backlog transitions through the queue logger. |
internal/connector/intake.go |
Documents intake’s logger adoption. |
internal/connector/queue_test.go |
Tests reporting before a panicking callback. |
internal/connector/seam_contract_test.go |
Tests ordered edge reporting through intake. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
The queue between intake and admission decides four edges: the backlog crossing its warning depth, an offer waiting for room (which stops the feed read), that wait ending, and the backlog falling back below the warning depth. It handed them only to optional callbacks, and runConnect sets none, so in production every edge went nowhere. The package says the pause is reported rather than hidden; it was hidden. The queue now reports each edge on its logger before running any callback, with the depth and the threshold crossed, and says plainly when reading the feed is paused and why. Intake already adopts the connector's logger for the queue, so every caller that builds intake gets the report and none can forget to wire it. A callback that panics is contained after the report, so it cannot silence it.
A backlog hovering at the warning depth wrote a warning and a recovery for every event, and a saturated queue wrote a pause and a resume for every event the feed delivered. A warning now clears at half the warning depth, and a pause is reported over only once no offer waits and the backlog has drained to half the pause depth. The feed's blocking at capacity is unchanged; only the reported edges move. The pause line now reports what the queue holds, not counting the offer waiting for room, and both warning lines point at the usual cause and at basecamp connect status.
a52d4d7 to
8b907e8
Compare
|
Rebased onto current main and verified again. Full local CI passes; focused race checks pass over 20 runs. Review corrections now describe a blocked queue producer without claiming the live feed itself stopped, keep the reported held depth at capacity under concurrent offers, and wait for asynchronous callback delivery in the test. Current head: d7d5587. No new harness. |
|
Ready for review on d7d5587: local and hosted CI are green, Copilot approved this exact head, and the review threads are resolved. Cubic could not provide a fresh pass because its monthly quota is exhausted. One scope finding was explicitly declined: general logging-handler panic containment is not added; the existing contract and tests are callback-specific, as explained at #820 (comment). No unanswered review findings remain. |
|
🤖 Local review at d7d5587: clean. CI is green on it (27 passed, 4 skipped, 1 neutral). Nothing new beyond what Copilot and cubic already took you through. I read |
Summary
Make a growing connector backlog visible even when no reporting callbacks are installed. Warn, pause, resume, and recovery transitions are logged with their depth and thresholds.
Warnings and pauses clear only after the backlog drains to half their thresholds, avoiding a stream of repeated messages around a boundary. Pausing describes a producer waiting for room, not necessarily the live feed: retries and recovery walks also offer work. The held depth is the capacity observed by the blocked offer, not a counter that could include other in-flight offers.
Verification
bin/cipasses ond7d55874bcde21bc4f3cb68ca9591e7eb5da7d6f.The panic-containment contract remains callback-specific; this PR does not introduce general fault tolerance around application logging handlers.