Repository navigation
feat: adopt timezone-aware timestamps - #67
nickbaxter18 wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| def snapshot(self) -> MonitoringSnapshot: | ||
| latency_stats = { | ||
| label: { | ||
| "count": float(len(samples)), | ||
| "avg": float(mean(samples)) if samples else 0.0, | ||
| "p95": float(_percentile(samples, 0.95)), | ||
| } | ||
| for label, samples in self._latencies.items() | ||
| } | ||
| queue_depth: Dict[str, float] = {} | ||
| if self._queue_samples: | ||
| samples = list(self._queue_samples) | ||
| queue_depth = { | ||
| "latest": float(samples[-1]), | ||
| "max": float(max(samples)), | ||
| "avg": float(mean(samples)), | ||
| } |
There was a problem hiding this comment.
[P1] Snapshot mutates shared metrics without locking
The new latency and queue-depth metrics are appended under a lock (record_latency/record_queue_depth), but snapshot() iterates over _latencies and _queue_samples without taking the same lock. If a request records latency while another thread calls metrics() or /metrics, the dictionary or deque can be mutated during iteration, causing RuntimeError: dictionary changed size during iteration or inconsistent aggregates. Consider taking _lock while copying these structures or snapshotting them under the lock.
Useful? React with 👍 / 👎.
| with ProcessPoolExecutor(max_workers=worker_count) as executor: | ||
| tasks = [ | ||
| asyncio.create_task(_submit(executor, spec, cfg)) | ||
| for spec, cfg in zip(specs, settings, strict=False) | ||
| ] | ||
| results = await asyncio.gather(*tasks, return_exceptions=True) |
There was a problem hiding this comment.
[P1] Process sandbox ignores timeout
In the sandboxed execution path the plugins are scheduled via ProcessPoolExecutor, but the executor is used as a context manager with the default shutdown(wait=True) (lines 264‑269). When _submit() raises asyncio.TimeoutError, the underlying process continues running and shutdown waits for it to finish before returning. As a result, a hung plugin still blocks execute_plugins() indefinitely even though the timeout is reported, defeating the purpose of the timeout option. The executor should be shut down with cancel_futures=True or otherwise kill the workers after a timeout so the call can return promptly.
Useful? React with 👍 / 👎.
…ct-pipeline-xuy9hr
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from threading import Lock | ||
| from time import perf_counter | ||
| from typing import ContextManager, Dict, Iterator, List, Optional | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix the issue, simply delete the extraneous line on line 14: codex/implement-google-news-drive-sync-project-pipeline-xuy9hr. This line does not contribute any functionality or documentation and is, at best, a distraction and source of confusion. No imports or new definitions are needed, nor do surrounding lines require any alteration. Ensure that the code’s functional or logical behavior is not changed by this removal.
| @@ -11,7 +11,6 @@ | ||
| from threading import Lock | ||
| from time import perf_counter | ||
| from typing import ContextManager, Dict, Iterator, List, Optional | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr | ||
|
|
||
| from .utils import utcnow | ||
|
|
| def start_run(self) -> None: | ||
| with self._lock: | ||
| self._last_run_started = datetime.utcnow() | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix the problem, the statement codex/implement-google-news-drive-sync-project-pipeline-xuy9hr on line 68 of google-news-drive-sync/src/monitor.py should be removed. This would eliminate the invalid, ineffective statement and clean up the code. No other changes are necessary, as deleting this line will not impact the function or flow of the code.
| @@ -65,7 +65,6 @@ | ||
|
|
||
| def start_run(self) -> None: | ||
| with self._lock: | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr | ||
| self._last_run_started = utcnow() | ||
|
|
||
|
|
||
| @@ -74,7 +73,6 @@ | ||
| def complete_run(self, *, status: str = "success") -> None: | ||
| with self._lock: | ||
| self._runs += 1 | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr | ||
| self._last_run_completed = utcnow() | ||
| self._last_status = status | ||
|
|
| with self._lock: | ||
| self._runs += 1 | ||
| self._last_run_completed = datetime.utcnow() | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
The best way to fix this issue is to simply remove the line codex/implement-google-news-drive-sync-project-pipeline-xuy9hr from line 77 of google-news-drive-sync/src/monitor.py. This line has no effect and is, in fact, a confusing artifact that should not remain in production or development code. No replacement is needed; the function will remain logically identical, but cleaner.
No changes are required to any imports, definitions, or methods beyond deleting this line.
| @@ -74,7 +74,6 @@ | ||
| def complete_run(self, *, status: str = "success") -> None: | ||
| with self._lock: | ||
| self._runs += 1 | ||
| codex/implement-google-news-drive-sync-project-pipeline-xuy9hr | ||
| self._last_run_completed = utcnow() | ||
| self._last_status = status | ||
|
|
|
2 participants
Summary
utcnow()helper and apply it to cache, repository, and monitoring paths so timestamps remain timezone-awareTesting
https://chatgpt.com/codex/tasks/task_e_68d5900aacb08321bccd4a0898cc3271