fix: remove dead child_ids field from _VSCodeDiscoveryCache (#1174)#1176
Open
fix: remove dead child_ids field from _VSCodeDiscoveryCache (#1174)#1176
child_ids field from _VSCodeDiscoveryCache (#1174)#1176Conversation
The `child_ids` field was stored on every cache miss but never read on cache hits, causing dead memory overhead proportional to the number of session directories. This removes the field from the dataclass while keeping `child_ids` as a local variable in `_cached_discover_vscode_logs` where it is actually used. - Remove `child_ids: _ChildIds` from `_VSCodeDiscoveryCache` dataclass - Update docstring to remove mention of stored child_ids - Update tests to remove `child_ids` kwarg from constructor calls - Replace `cached.child_ids` assertion with checks on remaining fields Closes #1174 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Removes an unused _VSCodeDiscoveryCache.child_ids field from the VS Code log discovery cache to eliminate unnecessary per-entry memory overhead while keeping cache-hit behavior unchanged.
Changes:
- Dropped the
child_idsfield from_VSCodeDiscoveryCacheand stopped persisting it in_VSCODE_DISCOVERY_CACHE. - Updated
_VSCodeDiscoveryCachedocstring to reflect the simplified cached state. - Adjusted unit tests to stop constructing/asserting against the removed field and validate the remaining sentinel fields instead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/copilot_usage/vscode_parser.py | Removes the dead cached child_ids field and updates cache population + docstring accordingly. |
| tests/copilot_usage/test_vscode_parser.py | Updates cache-related tests to match the new dataclass shape and assert on newest_child_* instead of child_ids. |
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
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.
Summary
Remove the unused
child_idsfield from_VSCodeDiscoveryCachewhich was stored on every cache miss but never consulted on cache hits, causing silent memory overhead proportional to the number of session directories.Changes
src/copilot_usage/vscode_parser.py:child_ids: _ChildIdsfrom_VSCodeDiscoveryCachefrozen dataclasschild_ids=child_idsfrom the cache construction in_cached_discover_vscode_logschild_idsbeing retainedchild_idsas a local variable where it's used (no semantic change)_ChildIdstype alias (still used by_scan_child_idsand_newest_child_from_ids)tests/copilot_usage/test_vscode_parser.py:child_ids=cached.child_idskwarg fromtest_cache_invalidated_on_root_mtime_changeassert cached.child_ids == _scan_child_ids(tmp_path)with assertions onnewest_child_pathandnewest_child_idfieldschild_ids=frozenset()fromTestVSCodeDiscoveryCacheIsFrozenCloses #1174
Warning
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.orgrepo.anaconda.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.