Repository navigation
fix(maintenance): sweep response handles, rotate hook analytics (#4099) - #4128
Conversation
Expired response handles were swept only when an MCP server was built, and hook_analytics.jsonl was never rotated, so both grew for a daemon's uptime (#4099). Full maintenance ticks now sweep expired handles under the profile and every registered store, and a fully imported hook analytics file past 64 MiB is truncated under its append lock with the import cursor rewound.
|
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| cursor_key: &str, | ||
| consumed: ParseOffset, | ||
| ) { | ||
| match truncate_if_unchanged(path, consumed.byte_offset) { |
There was a problem hiding this comment.
🔴 New hook rows lost after interrupted rotation
If rotate_consumed_file stops after truncation, its old cursor survives while hooks refill the file. Once the file exceeds that cursor, import_source skips its new prefix permanently.
Learn more
A hook import uses a byte offset in the database to resume reading its JSONL file. Rotation truncates the file before resetting that offset, and the two operations cannot commit atomically. If the process exits or the reset fails between them, the old offset remains. When new writes refill the file beyond that offset before the next import, import_source_rotating_above resumes from the old offset, losing the new prefix. The same window allows another importer to read the growing file with the old offset and commit an incorrect frontier.
Example: The imported file ends at byte 67,108,865. Rotation empties it, but the process exits before rewinding the cursor. Hooks write 67,108,900 new bytes before the next import, which begins at byte 67,108,865 and drops almost all new rows.
Recommended fix: Persist a generation or rotation marker that the import path checks before applying the byte offset, with recovery from interruption and concurrent imports. Coordinate reset and truncation under an import-wide ownership protocol; test interruption and regrowth beyond the former offset.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if file.metadata()?.len() != consumed_end { | ||
| return Ok(false); | ||
| } | ||
| file.set_len(0)?; |
There was a problem hiding this comment.
🔴 Hook diagnostics disappear after rotation
When rotate_consumed_file empties a large file, diagnostics_payload_from_parts still derives hook figures from that file. The CLI and dashboard report no recent hooks or readiness despite their durable events remaining available.
Learn more
Hook diagnostics independently reads the trailing JSONL rows using read_hook_analytics_rows_at and computes counts, recent hooks, and readiness from those rows. The durable table holds the imported events, but the diagnostics hook figures do not reconstruct their original hook rows from it. Truncation therefore changes the visible diagnostics immediately after an import. The dashboard uses the same read-side helpers.
Example: A project has 11,000 recent hook rows in a 65 MiB JSONL file. After analytics sync imports and rotates it, the next diagnostics call reports zero hook calls, zero recent hooks and empty readiness input until new hooks arrive.
Recommended fix: Move the hook diagnostics read to durable analytics events before deleting their JSONL source, preserving the current window and project attribution semantics. Alternatively retain a bounded recent hook window in the file and advance the cursor consistently.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if continuation.is_none() && !self.cancellation.is_cancelled() { | ||
| let handle_roots = std::iter::once(profile_root) | ||
| .chain( | ||
| registered_projects | ||
| .iter() | ||
| .map(|(_, store)| store.data_root.as_path()), | ||
| ) | ||
| .map(|root| root.join(RESPONSE_HANDLES_DIRECTORY)) | ||
| .collect(); | ||
| sweep_response_handles(handle_roots, self.cancellation.clone()).await; |
There was a problem hiding this comment.
🟡 Expired handles accumulate when retention is disabled
When retention_maintenance_enabled is false, the maintenance tick never starts, so sweep_response_handles never runs. Removing the server's one-shot sweep leaves expired handles in active projects indefinitely.
Learn more
MaintenanceCoordinator::spawn returns without a retention task when session-LCM and observation retention are disabled and both orphan-store GC and compaction are unset. The only new handle sweep is inside that task. The previous sweep at MCP server construction has also been removed, so disabling those unrelated retention policies disables handle expiry cleanup too.
Example: An operator disables session and observation retention, configures no orphan GC or compaction, then runs MCP tools for several days. Expired response-handle files remain on disk indefinitely.
Recommended fix: Give response-handle expiry its own always-running maintenance owner or enable the maintenance tick independently of configurable retention policy; keep its cadence and cancellation separate from disabled retention work.
Was this helpful? React with 👍 or 👎 to provide feedback.
Part of #4099.
Two profile sidecars grew for as long as a daemon ran:
hook_analytics.jsonl. The file was append-only and never rotated. Every row it holds is already durable inanalytics_eventsonce imported. After an import that consumes the whole file, a file over 64 MiB is truncated under the same sidecar lock every append takes, and only when its length still equals the imported end. The durable cursor is then rewound to zero, so rows appended after rotation import once from the start. A held lock, a longer file, or an unfinished trailing row leaves the file for the next import.Both match step 3 of the storage redesign (#4118, "Growth stops now"). The 512 MiB response-handle cap and the move to a
telemetry.hook_eventstable belong to the redesign and are not in this change.Verification
analytics_bridge::tests::a_consumed_file_over_the_threshold_rotates_and_new_rows_import_oncerotates a consumed file, appends more rows than it held, and asserts all eight rows are stored exactly once.analytics_bridge::tests::a_file_under_the_threshold_or_with_an_unconsumed_tail_is_not_rotatedandtruncation_requires_the_exact_consumed_length_and_the_append_lockcover the refusals.daemon::maintenance::tests::a_full_tick_sweeps_expired_response_handles_from_the_profile_and_each_storeruns a real maintenance tick and asserts each root keeps only its fresh handle.tracedecay-applicationandtracedecay. Clippy andrustfmt_check_2024for every touched crate.