P-Diff Fixes + Link Deserialization Fix (Deadlock Fixes Clean)#672
Closed
data-coasys wants to merge 16 commits intodevfrom
Closed
P-Diff Fixes + Link Deserialization Fix (Deadlock Fixes Clean)#672data-coasys wants to merge 16 commits intodevfrom
data-coasys wants to merge 16 commits intodevfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces comprehensive chunked-diff handling improvements and enhanced observability across the p-diff-sync Holochain zomes and executors. Key changes include: a new sweettest suite validating 600-link chunked-diff scenarios (broadcast, pull, render), addition of public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Closing - will recreate as two separate PRs per Nico's instructions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
bootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/src/test_commit_pull.rs (3)
418-435: Excessiveprintln!explanations in test output.Lines 418–435 print a multi-paragraph explanation of the bug and fix every time the test runs. This is better suited for a doc comment on the test function (which already exists at lines 271–284) rather than runtime output. Consider removing or condensing to a single summary line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/src/test_commit_pull.rs` around lines 418 - 435, The test prints a long multi-paragraph explanation at runtime (the block of println! calls summarizing the bug/fix referencing handle_broadcast(), load_diff_from_entry(), update_current_revision(), and pull()), which is noisy; remove those runtime println! statements and either convert the content into a doc comment on the test function (where existing docs live around lines 271–284) or replace them with a single concise println! summary line (e.g., "Fix: swapped load_diff_from_entry() and update_current_revision() in handle_broadcast()") so the test output remains brief.
403-416: Test can never fail — both branches pass.The
if bob_pull_result.is_err()/elseblock accepts both outcomes, making this test always pass regardless of whether chunks propagated in time. This documents the intended behavior but doesn't reliably catch regressions.If the goal is to verify the fix, consider isolating the test by deferring
exchange_peer_infountil after Alice's commit and Bob's pull attempt (i.e., setnetwork: falseinitially), so chunks are guaranteed absent on Bob's DHT. Then theis_errbranch would always execute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/src/test_commit_pull.rs` around lines 403 - 416, The test currently accepts both outcomes because it allows network propagation, so change the setup to guarantee chunks are absent on Bob before his pull: configure the test to start with network disabled (e.g., set network: false or defer calling exchange_peer_info) so Alice commits, then call bob.pull() (check bob_pull_result) and assert that bob_current_after == bob_current_before in the is_err branch; only after asserting the error branch, enable networking or call exchange_peer_info and re-run the successful propagation assertions; reference bob_pull_result, bob_current_before, bob_current_after, exchange_peer_info, and the pull()/handle_broadcast() behavior to locate and modify the test setup and order of operations.
72-75: Duplicated peer-exchange retry loop.The 5-iteration
exchange_peer_info+sleep(2000)pattern is duplicated acrosstest_merge_fetch(lines 72–75) andtest_merge_fetch_deep(lines 178–181). Consider extracting to a helper inutils.rsalongsideawait_consistency.Also applies to: 178-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/src/test_commit_pull.rs` around lines 72 - 75, Extract the duplicated 5-iteration retry loop that calls conductors.exchange_peer_info().await followed by tokio::time::sleep(Duration::from_millis(2000)).await into a new helper function (e.g., await_peer_exchange or retry_peer_exchange) in utils.rs next to await_consistency; implement it to accept the conductors object (or a reference) and perform the same loop, then replace the loop in test_merge_fetch and test_merge_fetch_deep with a call to this new helper to eliminate duplication.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/render.rs (1)
30-49: Same per-entryinfo!verbosity concern aschunked_diffs.rs.Line 36 logs for every diff entry in the workspace. For perspectives with many diffs, this could produce significant output. The aggregate totals at line 51 are the most useful; consider
debug!for per-entry logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/render.rs` around lines 30 - 49, The per-entry logging inside render.rs currently uses info! when iterating workspace.entry_map (the for (hash, diff_node) loop) which is too verbose; change that info! call to debug! so per-entry additions_count/removals_count are emitted at debug level while keeping the aggregate totals (total_additions, total_removals) logging at info; locate the info! call that references additions_count, removals_count and hash and replace it with debug! to reduce noise while leaving the rest of the logic (pushing additions into perspective.links and inserting removals into removals_set) unchanged.bootstrap-languages/p-diff-sync/linksAdapter.ts (1)
235-239: Minor: log output for null targets will show"undefined...".When
link.data.targetisnullorundefined(which can happen),link?.data?.target?.substring(0, 60)evaluates toundefined, and the template literal producestarget='undefined...'. Not a bug, but mildly misleading in logs.🔧 Suggestion
- console.log(`[p-diff-sync] 📤 commit(): addition[${i}]: source='${link?.data?.source}', pred='${link?.data?.predicate}', target='${link?.data?.target?.substring(0, 60)}...'`); + console.log(`[p-diff-sync] 📤 commit(): addition[${i}]: source='${link?.data?.source ?? '(null)'}', pred='${link?.data?.predicate ?? '(null)'}', target='${(link?.data?.target ?? '(null)').substring(0, 60)}...'`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/linksAdapter.ts` around lines 235 - 239, The log in commit() prints "undefined..." when link.data.target is null/undefined; update the logging to safely handle missing targets by using link (or link.data) and link.data.target checks and a fallback string (e.g., '<null>' or empty string) before calling substring — reference the commit() loop and the link variable and replace the inline link?.data?.target?.substring(0, 60) expression with a guarded expression that only calls substring when target is a string and uses a clear fallback otherwise so logs show "<null>" (or similar) instead of "undefined...".bootstrap-languages/p-diff-sync/index.ts (1)
39-58: Verbose per-signal diagnostic logging may need to be removed/guarded before release.Every signal (including frequent
syncbroadcasts) triggers JSON.stringify operations and multipleconsole.logcalls at lines 44-58. This is useful for debugging but could measurably impact throughput and log volume in production. Consider gating behind a debug flag or removing once the chunking issues are confirmed fixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/index.ts` around lines 39 - 58, The per-signal diagnostic logging in the signal handler (references: signal, payload, isArray, payloadKeys and the multiple console.log calls) is unguarded and will impact production throughput; change it so the heavy JSON.stringify and console.log blocks are executed only when a debug flag is enabled (e.g., check a DEBUG_P_DIFF_SYNC env/config or logger.isDebugEnabled()) or remove them for release — wrap the payload inspection and all console.log statements behind that check and use a proper logger debug level instead of unconditional console.log to avoid runtime and logging overhead in production.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/chunked_diffs.rs (1)
88-134: Consider reducing log level todebug!for per-chunk iteration logs.In
from_entries, every chunk produces 4info!calls (load start, load result, processing, processed). For a 10-chunk diff, that's 40info!-level log lines from this function alone. In a WASM zome, excessive logging adds measurable overhead. The start/complete logs (lines 88-91, 137-140) atinfo!are reasonable, but per-chunk logs could bedebug!.♻️ Suggestion: use debug! for per-chunk logs
for (idx, hash) in hashes.iter().enumerate() { - info!( + debug!( "ChunkedDiffs::from_entries: Loading chunk {}/{} (hash: {:?})", idx + 1, hashes.len(), hash ); let diff_entry = match Retreiver::get::<PerspectiveDiffEntryReference>(hash.clone()) { Ok(entry) => { - info!( + debug!( "ChunkedDiffs::from_entries: ✓ Chunk {}/{} retrieved successfully", idx + 1, hashes.len() ); entry }, Err(e) => { - info!( + debug!( "ChunkedDiffs::from_entries: ✗ FAILED to retrieve chunk {}/{} (hash: {:?}) - Error: {:?}", idx + 1, hashes.len(), hash, e );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/chunked_diffs.rs` around lines 88 - 134, Change the per-chunk iteration logs in ChunkedDiffs::from_entries from info! to debug! to reduce WASM logging overhead: specifically, switch the non-error info! calls inside the for loop that log "Loading chunk {}/...", the successful retrieval log ("✓ Chunk ... retrieved successfully"), the "Processing chunk ..." log and the "Chunk ... processed ..." log to debug!. Keep the outer start/complete info! logs and preserve the Err branch logs (the failed retrieve and "Chunks not available" messages) at info! so errors remain visible; locate these around Retreiver::get, diff_entry, and load_diff_from_entry to apply the changes.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/commit.rs (1)
83-119: Busy-wait spin loop for retry delay — consider the CPU cost and preserve the last error.Two concerns with the chunk verification retry:
Busy-wait (lines 110–115): The inner loop spins on
sys_time()to simulate a 100ms delay. In Holochain's WASM environment there may not be a better alternative, but 10 retries × 100ms of CPU spinning (up to 1 second total) is significant. If there's awasm_utils::sleepor similar available in the HDK, prefer that.Lost error context (lines 99–101): When retries are exhausted, the original error
eis discarded. Capturing it would aid debugging.Proposed fix for preserving error context
+ let mut last_error = None; loop { match Retriever::get::<PerspectiveDiffEntryReference>(chunk_hash.clone()) { Ok(_) => { info!("===PerspectiveDiffSync.commit(): Chunk {}/{} verified available", idx + 1, chunk_hashes.len()); break; } Err(e) => { retry_count += 1; if retry_count > MAX_RETRIES { - return Err(SocialContextError::InternalError( - "Failed to verify chunk availability after creation" - )); + info!( + "===PerspectiveDiffSync.commit(): Chunk {}/{} verification FAILED after {} retries. Last error: {:?}", + idx + 1, chunk_hashes.len(), MAX_RETRIES, e + ); + return Err(SocialContextError::InternalError( + "Failed to verify chunk availability after creation" + )); } + last_error = Some(e); info!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/commit.rs` around lines 83 - 119, Replace the busy-wait and lost-error behavior in the chunk verification loop: instead of spinning on sys_time() between attempts in commit, use an available async sleep (e.g., wasm_utils::sleep or HDK sleep) with RETRY_DELAY_MS to avoid CPU spin; also capture the original Retriever::get::<PerspectiveDiffEntryReference> error (store e) and when retry_count > MAX_RETRIES return a SocialContextError::InternalError that includes or wraps that original error message so failure context is preserved; keep MAX_RETRIES and RETRY_DELAY_MS constants but remove the inner sys_time spin loop and reference sys_time only if no sleep alternative exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/lib.rs`:
- Around line 134-139: The failure branch in recv_remote_signal logs a handling
failure with info!; change that to a proper higher severity (warn! or error!) —
update the Err(err) arm that contains info!("===recv_remote_signal: ✗
HashBroadcast handling FAILED - Error: {:?}", err) to use warn! (or error!)
instead, keeping the same formatted message and returning
Err(utils::err(&format!("{}", err))) unchanged.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/pull.rs`:
- Around line 266-268: The log shows revision == current_revision.hash but
execution continues; inside PerspectiveDiffSync::handle_broadcast() add an
explicit early return immediately after the info!() when revision equals
current_revision.hash to avoid running the parent-match and downstream work (use
the function's success return form, e.g. return Ok(()) or return Ok(None)
depending on the function signature). If instead you intend to always
re-broadcast at the later re-broadcast step (around the code referenced at line
312), make that behavior explicit with a comment and keep the early return
conditional only when you do NOT want re-broadcasting.
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 474-503: The per-link info logs in the pending-diffs commit path
expose raw link data (see the loop that iterates pending_diffs.additions and the
log::info! that prints link.data.source, link.data.predicate, link.data.target);
change that per-link log to a debug (or trace) level (e.g., log::debug!) or
redact sensitive fields, so only the summary log (the existing info-level line
that logs pending_ids.len() and additions/removals counts) remains at info;
leave the surrounding commit_result handling (commit, Ok(Some(rev)), Err(e), and
the call to self.set_immediate_commits) unchanged.
---
Duplicate comments:
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 663-713: The code currently logs full per-link contents at info
level in the commit path (see the loop that logs link.data.source,
link.data.predicate, link.data.target for each addition), which duplicates
earlier sensitive/verbose logs; change these per-link info logs to either debug
level or redact sensitive fields (e.g., log only link indices and types or
hashed/masked source/target) so you avoid duplicate high-volume
privacy-sensitive output while preserving the overall info-level summary (the
existing log::info with counts and the commit outcome via link_language.commit
remain fine).
- Around line 759-771: The current spawn block logs raw link/diff contents at
info level (log::info! and log::error! with {:?}) which duplicates
sensitive/verbose data; update the logging in the tokio::spawn closure (the
log::info! that prints "SPAWN COMMIT: Spawning commit..." and the log::error! /
log::info! that print commit() results) to avoid dumping raw diff
contents—either remove per-link/raw diff details, redact them (e.g. log counts
only like diff.additions.len() and diff.removals.len()), or lower the level to
debug/trace; ensure the messages in the async move block that reference
self_clone.commit(&diff_clone) do not interpolate full diff_clone data.
- Around line 3155-3156: The info-level log emitting raw command/expression data
(the log::info! call that prints commands.len(), expression, batch_id and
references the commands collection) risks privacy and high volume; change it to
a lower verbosity and/or redact sensitive fields — e.g., replace the log::info!
with log::debug! or log::trace! and stop printing the raw expression/commands
(only log commands.len() and batch_id), or mask the expression before logging;
update the statement that references commands.len(), expression, batch_id
accordingly to avoid dumping raw command contents.
- Around line 3453-3525: The code is logging raw command and value data at info
level (sensitive/verbose); change those info logs to a lower level or redact
sensitive fields before emitting. Specifically, in the create flow around
get_constructor_actions, get_property_setter_actions and resolve_property_value
where you log initial_values, each Command in commands, and command.target,
replace log::info! with log::debug! (or sanitize/mask command.target and
initial_values) so detailed payloads are not recorded at info level; ensure the
final completion/info logs remain high-level (e.g., keep timing and counts)
while detailed command/value dumps are either removed or only emitted at debug
level.
- Around line 1099-1100: The info-level log line that prints per-link raw
content and counts (the log::info! call starting "🔗 ADD LINK EXPR (batch):
added to batch=...") duplicates sensitive/verbose data; change it to a
lower-verbosity log (log::debug!) and remove or redact any raw link payloads so
only non-sensitive metadata remains (e.g., batch_id, status, and counts). Also
search for other similar per-link info logs in perspective_instance.rs and
convert them to debug/redacted forms to avoid duplicate noisy/privacy-leaking
logs.
- Around line 777-847: The per-link info-level logging that prints raw link
contents (the log::info! inside the additions loop that logs addition[{}]:
source='{}', pred={:?}, target='{}', author='{}') should be removed or reduced
to avoid sensitive/high-volume output; change that specific log to a lower level
(e.g., log::debug!) or redact sensitive fields (source/target/author) and only
emit non-sensitive metadata (index and counts). Update the loop that iterates
diff.additions (and similarly any equivalent loop over diff.removals) to use
log::debug! or a redacted message, keeping the overall high-level info logs (the
summary count logs using perspective_uuid and the post-dedup counts) unchanged.
- Around line 1008-1010: The info-level log statement in log::info! inside
perspective_instance (publishing diff) exposes raw owners and full
addition/removal counts/IDs (handle.uuid, decorated_diff.additions.len(),
decorated_diff.removals.len(), handle.owners) which duplicates sensitive/verbose
data — change it to redact or reduce detail and lower verbosity: replace the
log::info! with either log::debug! or log::trace! and/or log only safe
summarized fields (e.g., handle.uuid and numeric counts but not handle.owners or
raw IDs), or redact owner identifiers before logging; update the call site that
references decorated_diff and handle to log the sanitized values instead.
- Around line 919-920: The info-level log in perspective_instance.rs that prints
raw per-link contents via log::info!("🔗 ADD LINK: source='{}', pred={:?},
target='{}', status={:?}, batch_id={:?}", link.source, link.predicate,
link.target, status, batch_id) should be reduced or redacted to avoid
high-volume/privacy leaks; change it to a lower-severity log (debug/trace) or
sanitize fields (e.g., redact link.source and link.target or replace with
hashed/partial identifiers) and keep only non-sensitive metadata such as
predicate, status, and batch_id in the log; update the call site that constructs
this message so functionally identical contexts use the new format/severity.
- Around line 3408-3437: The info-level logs in the resolve-property flow expose
raw resolved values (see the log::info that prints result in the resolution
branch and other raw-value info/warn messages around JS_CORE_HANDLE) which is a
privacy/verbosity concern; change those value-displaying info logs to
debug-level (log::debug) or redact the actual content (e.g., mask result with
"[REDACTED]" or log only its size/hashed fingerprint) inside the resolve block
where prolog_query_with_context / prolog_get_first_string_binding and the
JS_CORE_HANDLE execution occur, and ensure the fallback path (the JS_CORE_HANDLE
None branch) does not emit raw value text at info level but instead logs a
non-sensitive message or debug/redacted content.
- Around line 4261-4377: The per-link info-level logging in the batch commit
loop (the log::info call that prints "🔄 BATCH COMMIT: addition[{}]:
source='{}', pred={:?}, target='{}', status={:?}" inside the for link in
diff.additions loop) leaks raw link contents and should be downgraded or
redacted; change that call to log::debug (or log::trace) and/or replace raw
fields with a redacted/summary representation (e.g. only predicate and status or
a helper like redact_link_display(link.data) that masks source/target) so full
source/target values are not logged at info level. Ensure the same approach is
applied to any similar per-link logging in this commit path so only
non-sensitive summaries remain at info level.
---
Nitpick comments:
In
`@bootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/src/test_commit_pull.rs`:
- Around line 418-435: The test prints a long multi-paragraph explanation at
runtime (the block of println! calls summarizing the bug/fix referencing
handle_broadcast(), load_diff_from_entry(), update_current_revision(), and
pull()), which is noisy; remove those runtime println! statements and either
convert the content into a doc comment on the test function (where existing docs
live around lines 271–284) or replace them with a single concise println!
summary line (e.g., "Fix: swapped load_diff_from_entry() and
update_current_revision() in handle_broadcast()") so the test output remains
brief.
- Around line 403-416: The test currently accepts both outcomes because it
allows network propagation, so change the setup to guarantee chunks are absent
on Bob before his pull: configure the test to start with network disabled (e.g.,
set network: false or defer calling exchange_peer_info) so Alice commits, then
call bob.pull() (check bob_pull_result) and assert that bob_current_after ==
bob_current_before in the is_err branch; only after asserting the error branch,
enable networking or call exchange_peer_info and re-run the successful
propagation assertions; reference bob_pull_result, bob_current_before,
bob_current_after, exchange_peer_info, and the pull()/handle_broadcast()
behavior to locate and modify the test setup and order of operations.
- Around line 72-75: Extract the duplicated 5-iteration retry loop that calls
conductors.exchange_peer_info().await followed by
tokio::time::sleep(Duration::from_millis(2000)).await into a new helper function
(e.g., await_peer_exchange or retry_peer_exchange) in utils.rs next to
await_consistency; implement it to accept the conductors object (or a reference)
and perform the same loop, then replace the loop in test_merge_fetch and
test_merge_fetch_deep with a call to this new helper to eliminate duplication.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/chunked_diffs.rs`:
- Around line 88-134: Change the per-chunk iteration logs in
ChunkedDiffs::from_entries from info! to debug! to reduce WASM logging overhead:
specifically, switch the non-error info! calls inside the for loop that log
"Loading chunk {}/...", the successful retrieval log ("✓ Chunk ... retrieved
successfully"), the "Processing chunk ..." log and the "Chunk ... processed ..."
log to debug!. Keep the outer start/complete info! logs and preserve the Err
branch logs (the failed retrieve and "Chunks not available" messages) at info!
so errors remain visible; locate these around Retreiver::get, diff_entry, and
load_diff_from_entry to apply the changes.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/commit.rs`:
- Around line 83-119: Replace the busy-wait and lost-error behavior in the chunk
verification loop: instead of spinning on sys_time() between attempts in commit,
use an available async sleep (e.g., wasm_utils::sleep or HDK sleep) with
RETRY_DELAY_MS to avoid CPU spin; also capture the original
Retriever::get::<PerspectiveDiffEntryReference> error (store e) and when
retry_count > MAX_RETRIES return a SocialContextError::InternalError that
includes or wraps that original error message so failure context is preserved;
keep MAX_RETRIES and RETRY_DELAY_MS constants but remove the inner sys_time spin
loop and reference sys_time only if no sleep alternative exists.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/render.rs`:
- Around line 30-49: The per-entry logging inside render.rs currently uses info!
when iterating workspace.entry_map (the for (hash, diff_node) loop) which is too
verbose; change that info! call to debug! so per-entry
additions_count/removals_count are emitted at debug level while keeping the
aggregate totals (total_additions, total_removals) logging at info; locate the
info! call that references additions_count, removals_count and hash and replace
it with debug! to reduce noise while leaving the rest of the logic (pushing
additions into perspective.links and inserting removals into removals_set)
unchanged.
In `@bootstrap-languages/p-diff-sync/index.ts`:
- Around line 39-58: The per-signal diagnostic logging in the signal handler
(references: signal, payload, isArray, payloadKeys and the multiple console.log
calls) is unguarded and will impact production throughput; change it so the
heavy JSON.stringify and console.log blocks are executed only when a debug flag
is enabled (e.g., check a DEBUG_P_DIFF_SYNC env/config or
logger.isDebugEnabled()) or remove them for release — wrap the payload
inspection and all console.log statements behind that check and use a proper
logger debug level instead of unconditional console.log to avoid runtime and
logging overhead in production.
In `@bootstrap-languages/p-diff-sync/linksAdapter.ts`:
- Around line 235-239: The log in commit() prints "undefined..." when
link.data.target is null/undefined; update the logging to safely handle missing
targets by using link (or link.data) and link.data.target checks and a fallback
string (e.g., '<null>' or empty string) before calling substring — reference the
commit() loop and the link variable and replace the inline
link?.data?.target?.substring(0, 60) expression with a guarded expression that
only calls substring when target is a string and uses a clear fallback otherwise
so logs show "<null>" (or similar) instead of "undefined...".
Comment on lines
+134
to
+139
| Err(err) => { | ||
| info!( | ||
| "===recv_remote_signal: ✗ HashBroadcast handling FAILED - Error: {:?}", | ||
| err | ||
| ); | ||
| Err(utils::err(&format!("{}", err))) |
Contributor
There was a problem hiding this comment.
Error logged at info! level — should be warn! or error!.
Line 135 logs a handling failure ("✗ HashBroadcast handling FAILED") using info!. Failures should use warn! or error! for proper log-level filtering in production.
🔧 Proposed fix
Err(err) => {
- info!(
+ error!(
"===recv_remote_signal: ✗ HashBroadcast handling FAILED - Error: {:?}",
err
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Err(err) => { | |
| info!( | |
| "===recv_remote_signal: ✗ HashBroadcast handling FAILED - Error: {:?}", | |
| err | |
| ); | |
| Err(utils::err(&format!("{}", err))) | |
| Err(err) => { | |
| error!( | |
| "===recv_remote_signal: ✗ HashBroadcast handling FAILED - Error: {:?}", | |
| err | |
| ); | |
| Err(utils::err(&format!("{}", err))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/lib.rs`
around lines 134 - 139, The failure branch in recv_remote_signal logs a handling
failure with info!; change that to a proper higher severity (warn! or error!) —
update the Err(err) arm that contains info!("===recv_remote_signal: ✗
HashBroadcast handling FAILED - Error: {:?}", err) to use warn! (or error!)
instead, keeping the same formatted message and returning
Err(utils::err(&format!("{}", err))) unchanged.
Comment on lines
266
to
268
| if revision == current_revision.hash { | ||
| // debug!("===PerspectiveDiffSync.fast_forward_signal(): Revision is the same as current"); | ||
| info!("===PerspectiveDiffSync.handle_broadcast(): Revision is the same as current - no action needed"); | ||
| }; |
Contributor
There was a problem hiding this comment.
Missing early return when broadcast revision matches current.
Line 267 logs "no action needed" but execution falls through to the parent-matching check on line 270. If this is the same revision, we should return early to avoid unnecessary work. If the intention is to always re-broadcast (line 312), consider making that explicit.
Proposed fix
if revision == current_revision.hash {
- info!("===PerspectiveDiffSync.handle_broadcast(): Revision is the same as current - no action needed");
- };
+ info!("===PerspectiveDiffSync.handle_broadcast(): Revision is the same as current - no action needed");
+ // Still emit the broadcast signal for other peers, but skip processing
+ emit_signal(broadcast)?;
+ let fn_end = get_now()?.time();
+ info!(
+ "===PerspectiveDiffSync.handle_broadcast(): COMPLETE (no-op) - Took: {}ms",
+ (fn_end - fn_start).num_milliseconds()
+ );
+ return Ok(());
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if revision == current_revision.hash { | |
| // debug!("===PerspectiveDiffSync.fast_forward_signal(): Revision is the same as current"); | |
| info!("===PerspectiveDiffSync.handle_broadcast(): Revision is the same as current - no action needed"); | |
| }; | |
| if revision == current_revision.hash { | |
| info!("===PerspectiveDiffSync.handle_broadcast(): Revision is the same as current - no action needed"); | |
| // Still emit the broadcast signal for other peers, but skip processing | |
| emit_signal(broadcast)?; | |
| let fn_end = get_now()?.time(); | |
| info!( | |
| "===PerspectiveDiffSync.handle_broadcast(): COMPLETE (no-op) - Took: {}ms", | |
| (fn_end - fn_start).num_milliseconds() | |
| ); | |
| return Ok(()); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/pull.rs`
around lines 266 - 268, The log shows revision == current_revision.hash but
execution continues; inside PerspectiveDiffSync::handle_broadcast() add an
explicit early return immediately after the info!() when revision equals
current_revision.hash to avoid running the parent-match and downstream work (use
the function's success return form, e.g. return Ok(()) or return Ok(None)
depending on the function signature). If instead you intend to always
re-broadcast at the later re-broadcast step (around the code referenced at line
312), make that behavior explicit with a comment and keep the early return
conditional only when you do NOT want re-broadcasting.
Comment on lines
+474
to
503
| log::info!("📤 PENDING DIFFS: Committing {} pending diffs ({} additions, {} removals) for perspective {}", | ||
| pending_ids.len(), pending_diffs.additions.len(), pending_diffs.removals.len(), uuid); | ||
| for (i, link) in pending_diffs.additions.iter().enumerate() { | ||
| log::info!("📤 PENDING DIFFS: addition[{}]: source='{}', pred={:?}, target='{}'", | ||
| i, link.data.source, link.data.predicate, link.data.target); | ||
| } | ||
| let commit_result = link_language.commit(pending_diffs).await; | ||
| match commit_result { | ||
| Ok(Some(_)) => { | ||
| Ok(Some(rev)) => { | ||
| Ad4mDb::with_global_instance(|db| { | ||
| db.clear_pending_diffs(&uuid, pending_ids) | ||
| })?; | ||
| // Reset immediate commits counter after successful commit | ||
| self.set_immediate_commits(IMMEDIATE_COMMITS_COUNT).await; | ||
| log::info!("Successfully committed pending diffs"); | ||
| log::info!("📤 PENDING DIFFS: Successfully committed, revision={}, reset immediate_commits to {}", | ||
| rev, IMMEDIATE_COMMITS_COUNT); | ||
| Ok(()) | ||
| } | ||
| Ok(None) => Err(anyhow!("No diff returned from commit")), | ||
| Err(e) => Err(e), | ||
| Ok(None) => { | ||
| log::warn!("📤 PENDING DIFFS: Commit returned None revision"); | ||
| Err(anyhow!("No diff returned from commit")) | ||
| } | ||
| Err(e) => { | ||
| log::error!("📤 PENDING DIFFS: Commit failed: {:?}", e); | ||
| Err(e) | ||
| } | ||
| } | ||
| } else { | ||
| log::warn!("📤 PENDING DIFFS: No link language available, keeping {} diffs for later", pending_ids.len()); | ||
| Ok(()) // Keep diffs if no link language |
Contributor
There was a problem hiding this comment.
Downgrade per-link info logs to avoid PII/volume exposure.
Info-level logs include raw source/predicate/target for every pending diff; this can leak user data and overwhelm logs on bursts. Keep count-only at info and move per-link details to debug/trace (or redact).
Proposed fix (move per-link details to debug)
- log::info!("📤 PENDING DIFFS: Committing {} pending diffs ({} additions, {} removals) for perspective {}",
- pending_ids.len(), pending_diffs.additions.len(), pending_diffs.removals.len(), uuid);
- for (i, link) in pending_diffs.additions.iter().enumerate() {
- log::info!("📤 PENDING DIFFS: addition[{}]: source='{}', pred={:?}, target='{}'",
- i, link.data.source, link.data.predicate, link.data.target);
- }
+ log::info!(
+ "📤 PENDING DIFFS: Committing {} pending diffs ({} additions, {} removals) for perspective {}",
+ pending_ids.len(),
+ pending_diffs.additions.len(),
+ pending_diffs.removals.len(),
+ uuid
+ );
+ if log::log_enabled!(log::Level::Debug) {
+ for (i, link) in pending_diffs.additions.iter().enumerate() {
+ log::debug!(
+ "📤 PENDING DIFFS: addition[{}]: source='{}', pred={:?}, target='{}'",
+ i, link.data.source, link.data.predicate, link.data.target
+ );
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/perspectives/perspective_instance.rs` around lines 474 -
503, The per-link info logs in the pending-diffs commit path expose raw link
data (see the loop that iterates pending_diffs.additions and the log::info! that
prints link.data.source, link.data.predicate, link.data.target); change that
per-link log to a debug (or trace) level (e.g., log::debug!) or redact sensitive
fields, so only the summary log (the existing info-level line that logs
pending_ids.len() and additions/removals counts) remains at info; leave the
surrounding commit_result handling (commit, Ok(Some(rev)), Err(e), and the call
to self.set_immediate_commits) unchanged.
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
2 participants
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
This PR contains the fixes from the
deadlock-fixesbranch, without the deadlock fix commits that are no longer needed now that PR #652 (SurrealDB file-based storage) is merged.Changes Included
P-Diff Chunking Fixes
Tests
Debug Logging
Critical Fix
Commits Intentionally Excluded
The following deadlock fix attempts from the original
deadlock-fixesbranch were intentionally excluded as they are no longer needed after merging PR #652:a947ca42- wip: deadlock fix126cd46c- wip: deadlock fixes, deactivate prolog subscriptions914a729b- fix deadlock in ensure_engine_updated()Created By
Data (AI Assistant) at Nico's request while he was driving home.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Chores