Fix #3051: optional query log rotation via GRAPHIFY_QUERY_LOG_MAX_RECORDS - #3244
Fix #3051: optional query log rotation via GRAPHIFY_QUERY_LOG_MAX_RECORDS#3244akshitj11 wants to merge 3 commits into
Conversation
Bound live JSONL size when GRAPHIFY_QUERY_LOG_MAX_RECORDS is set. Overflow records append to a sibling archive file. Default off.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Formal verification. 4 change(s) tested, no difference found (not proven).
Graphify review — findings
Adds optional query-log rotation controlled by GRAPHIFY_QUERY_LOG_MAX_RECORDS: when set to a positive integer, log_query trims the live log to the newest N records after each write and appends the overflow to a sibling .archive file. An unset, non-integer, or non-positive value leaves the log untrimmed, and rotation runs inside the existing swallow-all except so it never raises on failure.
Worth a look
- Non-atomic read-modify-write rotation races between concurrent processes —
graphify/querylog.py:58· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Unsynchronized rotation can drop concurrent log appends —
graphify/querylog.py:58· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Rotation can duplicate archived records after live-file replacement failure —
graphify/querylog.py:70· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 188 functions depend on the 40 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 117 callees - new:
log_query()— 20 callers, 5 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 188 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 46 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_check\_skill\_version.
The verifier did not have enough to check \_check\_skill\_version, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `skill_dst` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_run\_cli (not a proof).
The verifier ran both versions of \_run\_cli on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
No difference found (not proven): No behavior difference found in find\_import\_cycles (not a proof).
The verifier ran both versions of find\_import\_cycles on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_infer\_merge\_root.
The verifier did not have enough to check \_infer\_merge\_root, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify build\_merge.
The verifier did not have enough to check build\_merge, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify merge\_raw\_extraction.
The verifier did not have enough to check merge\_raw\_extraction, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify prefix\_graph\_for\_global.
The verifier did not have enough to check prefix\_graph\_for\_global, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 115 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous
Could not verify: Could not verify \_absolutize\_ids\_in.
The verifier did not have enough to check \_absolutize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_flush\_stat\_index (not a proof).
The verifier ran both versions of \_flush\_stat\_index on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_relativize\_ids\_in.
The verifier did not have enough to check \_relativize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_rewrite\_strings (not a proof).
The verifier ran both versions of \_rewrite\_strings on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
Serialize append and rotate under POSIX flock on the log lock file. Write the live log before appending overflow to the archive so a failed replace cannot duplicate archived records.
|
Addressed rotation advisories:
Windows remains best-effort without flock. The log_query coupling-delta note is acknowledged: rotation stays inside the existing hub with no API surface change. |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds optional size-based rotation to the query log, gated on GRAPHIFY_QUERY_LOG_MAX_RECORDS: once the live file exceeds that many records, _rotate_if_needed trims it to the newest N lines via an atomic os.replace and appends the overflow to a sibling .archive file. Serializes each append-and-rotate under an exclusive flock (_query_log_lock) so concurrent writers don't lose or corrupt records, degrading to best-effort on platforms without fcntl. Leaving the env var unset, invalid, or non-positive keeps the previous append-only behaviour, and all rotation work stays inside the existing fail-silent try, so a broken path never raises.
Worth a look
- Lock is a no-op when fcntl is unavailable —
graphify/querylog.py:62· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Lock sidecar failure prevents otherwise writable query log append —
graphify/querylog.py:69· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Rotation can silently drop overflow records if archive append fails —
graphify/querylog.py:88· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Predictable temp path follows symlinks during rotation —
graphify/querylog.py:89· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Rotation can broaden permissions on the live query log —
graphify/querylog.py:89· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 193 functions depend on the 45 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 117 callees - new:
log_query()— 23 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 193 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 51 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
Use paths._atomic_replace for live rewrites, degrade to unlocked append when the lock file cannot be opened, and re-append overflow to the live log if archive append fails after replace.
|
Round 2 hardening:
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Formal verification. 4 change(s) tested, no difference found (not proven).
Graphify review — findings
Adds optional size-based rotation to the query log, gated on GRAPHIFY_QUERY_LOG_MAX_RECORDS: when set to a positive int, log_query trims the live JSONL to the newest N records via an atomic replace and appends the overflow to a sibling .archive file, leaving behaviour unchanged when the var is unset or non-positive. Serializes append+rotate under a per-file flock on POSIX, degrading to an unlocked append (rather than dropping the line) when fcntl is absent or the lock file can't be opened. On archive-write failure the overflow lines are re-appended to the live log so nothing is lost, and the whole path stays fail-silent.
Worth a look
- Unlocked fallback allows concurrent rotations to lose appended records —
graphify/querylog.py:74· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- flock failure drops the log record —
graphify/querylog.py:81· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Non-atomic read-modify-replace in rotation can drop records under concurrent processes —
graphify/querylog.py:100· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Archive-failure fallback re-appends overflow after live already trimmed, corrupting order and duplicating on next rotation —
graphify/querylog.py:105· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Partial archive write duplicates overflow —
graphify/querylog.py:105· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 199 functions depend on the 51 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 117 callees - new:
log_query()— 25 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 199 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 57 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_check\_skill\_version.
The verifier did not have enough to check \_check\_skill\_version, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `skill_dst` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_run\_cli (not a proof).
The verifier ran both versions of \_run\_cli on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
No difference found (not proven): No behavior difference found in find\_import\_cycles (not a proof).
The verifier ran both versions of find\_import\_cycles on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_infer\_merge\_root.
The verifier did not have enough to check \_infer\_merge\_root, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify build\_merge.
The verifier did not have enough to check build\_merge, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify merge\_raw\_extraction.
The verifier did not have enough to check merge\_raw\_extraction, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify prefix\_graph\_for\_global.
The verifier did not have enough to check prefix\_graph\_for\_global, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 115 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous
Could not verify: Could not verify \_absolutize\_ids\_in.
The verifier did not have enough to check \_absolutize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_flush\_stat\_index (not a proof).
The verifier ran both versions of \_flush\_stat\_index on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_relativize\_ids\_in.
The verifier did not have enough to check \_relativize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_rewrite\_strings (not a proof).
The verifier ran both versions of \_rewrite\_strings on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 1 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
| fh.writelines(overflow) | ||
|
|
||
|
|
||
| def log_query( |
There was a problem hiding this comment.
log_query()
fans out to 6 callees (efferent coupling); 25 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
Fix #3051: optional query log rotation via GRAPHIFY_QUERY_LOG_MAX_RECORDS
Problem
graphify/querylog.py is append-only. Deployments that opt in with
GRAPHIFY_QUERY_LOG_ENABLE or GRAPHIFY_QUERY_LOG can accumulate unbounded
JSONL, especially when GRAPHIFY_QUERY_LOG_RESPONSES stores full responses.
Change
Tests
tests/test_querylog.py: trim/keep order, archive accumulation, invalid env noop,
default unchanged, never raises.
Closes #3051