Repository navigation
Identity activation follow-ups from #341 review - #345
schickling-assistant merged 2 commits into
Conversation
agent-identity: agent/dotfiles/fabric-mesh agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.12.5 agent-runtime: OMP 18.12.5 agent-model: anthropic/claude-opus-5-5
|
CI at bd45ec9 is green. The first macOS run failed only in the known timing test Posted on behalf of @schickling
|
|
Status from the Fractal side: compoundingtech/fractal#3644 now implements the one-command re-key on top of this PR (identity prepare/activate plus runtime_peers Posted on behalf of @schickling
|
|
@myobie this PR is ready for your review at head bd45ec9, and CI is green. It contains the follow-ups from your #341 review. Fractal re-key work (compoundingtech/fractal#3644) needs a v0.9.4 tag that includes these changes. Could you review it, or say when you expect to? Thank you. Posted on behalf of @schickling
|
|
Fabric-mesh ( Posted on behalf of @schickling
|
…2026-10-30-identity-review-followups338 # Conflicts: # CHANGELOG.md
|
Integrated main after #346 using a normal merge commit, without rebasing or force-pushing. New head: bc88770. The CHANGELOG retains both Fixed bullets exactly once. Diff against integrated main is now 237 changed lines: CHANGELOG +3, README +7, src/main.rs +184/-1, tests/json_output.rs +42. src/identity.rs is identical to main (same Git blob 172759e381a9a46871d68ea2e57af9f398d2bce8). Hosted CI watched to completion at this head:
Release preparation #347 has not been changed. Posted on behalf of @schickling
|
|
GO at bc88770 (Review346, an independent reviewer subagent). The merge of main (#346 fc34b89) is clean, and src/identity.rs equals main. The remainder (signal guard, recovery, JSON output, tests) has no findings. Hosted test run 41719459414 and Nix run 41719459276 passed. Fabric-mesh merges it under Johannes's self-merge authorization (issuecomment-6609960025). Posted on behalf of @schickling
|
Problem
Nathan's round-two review of #341 identified four follow-ups before 0.9.4: teardown-timeout recovery abandoned a stopped node, failed/crashed preparation could retain an unreferenced secret, operational read errors could replace a valid pending key, and termination signals could interrupt recovery after Shutdown.
Goal
Fix exactly those four items; no deferred #343 work.
Decisions
src/main.rs:3251: a reconciliation Shutdown returningdaemon_not_runningfalls through to the lifetime-lease barrier and restart; regression:identity_stop_timeout_recovery_waits_for_teardown_and_restarts_current(tests/json_output.rs:293) exercises actual activation, the 15-second stop timeout, delayed teardown, and successful old-identity restart while retaining the original coded error.src/identity.rs:86,115: remove failed private scratch writes and sweep abandoned.identity.next.*.tmpwhile holding the identity lock, including idempotent prepare; regressions:prepare_removes_private_scratch_after_sync_or_rename_failure,prepare_sweeps_crashed_scratch_only_while_locked_even_when_already_prepared.src/identity.rs:53,65: distinguish malformed TOML/UTF-8 from operational pending-key read errors. EMFILE/EIO/EACCES propagate through the existinginternalerror code without replacing either key; regressions:pending_read_io_failure_preserves_valid_key_and_propagates_existing_error_code,prepare_replaces_pending_only_when_absent_or_invalid.src/main.rs:3133,3181: scoped caught no-op handlers discard SIGINT/SIGTERM/SIGHUP through activation and awaited recovery/restart, restore prior dispositions afterward, and reset on exec rather than making children inherit SIG_IGN; regressions:identity_activation_survives_sigint_through_restart_and_recovery,identity_activation_survives_sigterm_through_restart_and_recovery,identity_activation_survives_sighup_through_restart_and_recovery. Each isolated subprocess receives its signal at Shutdown before rename and again during reconciliation/recovery; both completion and pre-rename recovery, disposition restoration, and exec-child termination are asserted.README and CHANGELOG describe these behaviors. No CLI, wire, dependency, or version changes.
Verification
Exact reviewed head:
bd45ec9cac98c217266855a696f772d364fe66a5.One serial heavy-gated run in the repository dev shell completed with
exit=0,clean=yes, andhead=bd45ec9cac98:Independent source reviews before push: correctness GO (confidence 0.94) and security GO, both at the full exact head above. Reviewers inspected all four changes and their regressions; they did not execute tests. Existing dead-code warnings remain outside this delta.
Hosted proof at this head: test and Nix were in progress at the initial status check (
public-textalready passed); no CI watch is running. The dedicated local Cargo target was removed after testing (3.9 GiB).Complexity
No dependencies. The scoped signal guard is the sole production lifecycle addition; I/O fault injection is test-only and thread-local.
Concerns
Catchable cancellation signals received during activation are intentionally discarded, not deferred. SIGKILL and machine failure remain governed by the existing crash/retry contract. Recovery remains bounded and best-effort if teardown exceeds its recovery deadline.
Friction & bottlenecks
The first local run passed but an empty temporary PR-body file caused dirty-worktree classification. Removed it and repeated all three filters to obtain the clean exact-head receipt above.
Follow-ups
Other #343 items remain unchanged and deferred.
References
Refs #341, #343. Implements Nathan's round-two review on #341, posted 2026-10-29 at 17:52:13 UTC.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimeagent_modelworktree