sd-future: rework cancellation, add slot callbacks and future groups by daandemeyer · Pull Request #15 · daandemeyer/systemd

daandemeyer · 2026-06-02T12:00:18Z

Cancellation propagation:

sd_fiber_resume() now stashes the resume value even when the target fiber isn't suspended yet (INITIAL or running/READY); the next fiber_swap() consumes it without yielding to the event loop. -ECANCELED is sticky once queued, so a concurrent async wakeup can't override a pending cancellation.
sd_future_cancel_wait_unref() remembers the last non-zero return from its internal sd_fiber_await() across loop iterations and, once the future has finally resolved, re-queues it via sd_fiber_resume(self, ...) so the calling fiber's next sd_fiber_suspend() / sd_fiber_yield() observes it. Previously those values were discarded.

Together these let sd_future_cancel_wait_unref() forward interruptions (cancellations / outer SD_FIBER_TIMEOUT firings) instead of silently swallowing them.

FIBER_STATE_CANCELLED is dropped in the process: fiber_cancel() now queues -ECANCELED through the same sd_fiber_resume() path (with an idempotency check so fiber_on_exit()'s two-pass arm-then-dispatch cycle still works), and fiber_swap() delivers it via the standard result_pending branch instead of a dedicated state-machine check. Net effect: one enum value gone, the cancel path goes through exactly the same plumbing as any other async wakeup.

Decouple unreferencing a future from resolving it: futures must now be resolved by the time they're unreffed. To make this easier we introduce sd_future_cancel_unref() (plus _unrefp / array variants), which cancels and then unrefs. We use it everywhere where we can't assume we're on a fiber and thus can't wait for the future to resolve after cancelling. All future kinds we support today except fibers cancel synchronously, so this isn't a problem in practice. Even fibers cancel synchronously after creation until they're scheduled once by the event loop, after which they cancel asynchronously.

Slot-based callbacks:

Replaces the single-callback-per-future model (sd_future_set_callback, sd_future_get_userdata, and the wait_future indirection in sd_fiber_await) with explicit, awaiter-owned slots:

sd_future_add_callback() returns a refcounted sd_future_slot the caller owns via cleanup(sd_future_slot_unrefp). A future can have many slots; dropping a slot deregisters that one callback cleanly without resolving or otherwise touching the future. NULL ret_slot makes the slot 'floating' — its lifetime is bound to the future.
Callbacks never run inline. Each slot owns a one-shot defer event source and a one-shot exit event source on the future's event loop; the one applicable to the loop's current phase is armed when the future resolves — or at add time when the slot is attached to an already-RESOLVED future. Slots inherit the calling fiber's priority when added from inside one. Deferring unconditionally keeps the resolver free of reentrancy concerns (slot-set mutation during iteration, callbacks dropping the future's last ref) and frees callers from having to reason about whether a callback might fire before sd_future_add_callback() returns.
sd_fiber_await() registers a slot on the target directly and drops it on scope exit — no more wait_future allocation, no auto-set callback on sd_future_new(). sd_fiber_timeout() and sd_fiber_sleep() likewise install their resume callback explicitly via sd_future_add_callback().
sd_future_new_wait() / sd_future_set_callback() / sd_future_get_userdata() are gone; existing callers in sd-bus, qmp-client, event-future, and fiber-io migrate to the slot API. sd_future_resume_callback() is exposed as a shared helper for the common "resume my fiber when this future settles" wiring.

sd_future_new() now takes the sd_event the future belongs to, and sd_future_get_event() exposes it. This lets the rest of the API (slot dispatch, group child tracking, defer futures) avoid plumbing the event through separately.

Future groups and defer futures:

Adds sd_future_group, modelled on asyncio.TaskGroup: aggregate N child futures with a policy (default = wait-all-fail-fast; SD_FUTURE_GROUP_WAIT_ANY resolves on the first settled child; SD_FUTURE_GROUP_IGNORE_ERRORS collects all results without cancelling siblings on error). Policy is configured up-front via sd_future_group_set_policy() before any child is added — once children are in flight, the resolution mechanics are locked in. On group failure, the parent fiber that created the group is cancelled so it observes the failure even if it hasn't started awaiting yet — and not cancelled when it's already inside sd_future_group_await(), nor when the parent itself is the fiber driving the cancel. Finalize-time draining ensures the group only resolves once every child has actually settled, even if siblings need a round-trip through the event loop to honor a cancel.

Also adds sd_future_new_defer() for one-shot 'resolve on next event loop iteration' futures.

Tests cover the queued-while-running behavior of sd_fiber_resume(), the propagation guarantees of sd_future_cancel_wait_unref() for both timeouts and external cancellations (from main and from a peer fiber), and a new test-future-group.c with coverage for each policy, parent-cancel propagation (including the parent-drives-cancel skip path), child-drain-on-resolve, add-rejected-during-finalize, slot lifecycle, and the already-resolved add_callback path.

Cancellation propagation: - sd_fiber_resume() now stashes the resume value even when the target fiber isn't suspended yet (INITIAL or running/READY); the next fiber_swap() consumes it without yielding to the event loop. -ECANCELED is sticky once queued, so a concurrent async wakeup can't override a pending cancellation. - sd_future_cancel_wait_unref() remembers the last non-zero return from its internal sd_fiber_await() across loop iterations and, once the future has finally resolved, re-queues it via sd_fiber_resume(self, ...) so the calling fiber's next sd_fiber_suspend() / sd_fiber_yield() observes it. Previously those values were discarded. Together these let sd_future_cancel_wait_unref() forward interruptions (cancellations / outer SD_FIBER_TIMEOUT firings) instead of silently swallowing them. FIBER_STATE_CANCELLED is dropped in the process: fiber_cancel() now queues -ECANCELED through the same sd_fiber_resume() path (with an idempotency check so fiber_on_exit()'s two-pass arm-then-dispatch cycle still works), and fiber_swap() delivers it via the standard result_pending branch instead of a dedicated state-machine check. Net effect: one enum value gone, the cancel path goes through exactly the same plumbing as any other async wakeup. Decouple unreferencing a future from resolving it: futures must now be resolved by the time they're unreffed. To make this easier we introduce sd_future_cancel_unref() (plus _unrefp / array variants), which cancels and then unrefs. We use it everywhere where we can't assume we're on a fiber and thus can't wait for the future to resolve after cancelling. All future kinds we support today except fibers cancel synchronously, so this isn't a problem in practice. Even fibers cancel synchronously after creation until they're scheduled once by the event loop, after which they cancel asynchronously. Slot-based callbacks: Replaces the single-callback-per-future model (sd_future_set_callback, sd_future_get_userdata, and the wait_future indirection in sd_fiber_await) with explicit, awaiter-owned slots: - sd_future_add_callback() returns a refcounted sd_future_slot the caller owns via _cleanup_(sd_future_slot_unrefp). A future can have many slots; dropping a slot deregisters that one callback cleanly without resolving or otherwise touching the future. NULL ret_slot makes the slot 'floating' — its lifetime is bound to the future. - Callbacks never run inline. Each slot owns a one-shot defer event source and a one-shot exit event source on the future's event loop; the one applicable to the loop's current phase is armed when the future resolves — or at add time when the slot is attached to an already-RESOLVED future. Slots inherit the calling fiber's priority when added from inside one. Deferring unconditionally keeps the resolver free of reentrancy concerns (slot-set mutation during iteration, callbacks dropping the future's last ref) and frees callers from having to reason about whether a callback might fire before sd_future_add_callback() returns. - sd_fiber_await() registers a slot on the target directly and drops it on scope exit — no more wait_future allocation, no auto-set callback on sd_future_new(). sd_fiber_timeout() and sd_fiber_sleep() likewise install their resume callback explicitly via sd_future_add_callback(). - sd_future_new_wait() / sd_future_set_callback() / sd_future_get_userdata() are gone; existing callers in sd-bus, qmp-client, event-future, and fiber-io migrate to the slot API. sd_future_resume_callback() is exposed as a shared helper for the common "resume my fiber when this future settles" wiring. sd_future_new() now takes the sd_event the future belongs to, and sd_future_get_event() exposes it. This lets the rest of the API (slot dispatch, group child tracking, defer futures) avoid plumbing the event through separately. Future groups and defer futures: Adds sd_future_group, modelled on asyncio.TaskGroup: aggregate N child futures with a policy (default = wait-all-fail-fast; SD_FUTURE_GROUP_WAIT_ANY resolves on the first settled child; SD_FUTURE_GROUP_IGNORE_ERRORS collects all results without cancelling siblings on error). Policy is configured up-front via sd_future_group_set_policy() before any child is added — once children are in flight, the resolution mechanics are locked in. On group failure, the parent fiber that created the group is cancelled so it observes the failure even if it hasn't started awaiting yet — and not cancelled when it's already inside sd_future_group_await(), nor when the parent itself is the fiber driving the cancel. Finalize-time draining ensures the group only resolves once every child has actually settled, even if siblings need a round-trip through the event loop to honor a cancel. Also adds sd_future_new_defer() for one-shot 'resolve on next event loop iteration' futures. Tests cover the queued-while-running behavior of sd_fiber_resume(), the propagation guarantees of sd_future_cancel_wait_unref() for both timeouts and external cancellations (from main and from a peer fiber), and a new test-future-group.c with coverage for each policy, parent-cancel propagation (including the parent-drives-cancel skip path), child-drain-on-resolve, add-rejected-during-finalize, slot lifecycle, and the already-resolved add_callback path.

github-actions · 2026-06-02T12:16:59Z

+        if (r < 0)
+                return r;
+
+        return sd_future_result(target);


Claude: suggestion: After sd_fiber_suspend() returns a non-negative value, sd_fiber_await() now returns sd_future_result(target). If the fiber was woken by something other than target resolving (e.g. an unrelated sd_fiber_resume(self, 0) wired up by the caller), target is still PENDING and sd_future_result() will log an assert_return failure and return -EBUSY. The previous sd_fiber_suspend() path returned the resume value directly without this trap. This also feeds back into sd_future_cancel_wait_unref(), whose loop calls sd_fiber_await(f): a spurious non-negative wakeup turns into q = -EBUSY, which then gets re-queued onto the calling fiber as a bogus interruption. Consider guarding on sd_future_state(target) == SD_FUTURE_RESOLVED before returning its result, or returning the raw suspend value when the target hasn't resolved.

daandemeyer · 2026-07-13T09:00:31Z

@claude review

daandemeyer · 2026-07-13T09:03:32Z

@claude review

daandemeyer · 2026-07-13T09:20:22Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Pre-create the Git config mount target required by Claude's read-only sandbox so it can fetch PR context itself. A manual run on fork PR #15 covered the repository guard, AWS OIDC authentication, Opus model invocation, structured artifact handoff, and the post job. It exposed the missing sandbox mount target; rerun the workflow to verify that Claude can now inspect the PR diff.

daandemeyer · 2026-07-13T10:06:26Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Allow ephemeral checkout writes because Claude's Linux sandbox installs mandatory-deny bind mounts there; GitHub access remains read-only and the separate post job still validates all model output. Manual runs on fork PR #15 covered the repository guard, AWS OIDC authentication, and Opus invocation. They also exposed the sandbox-runtime failures on missing .gitconfig and .gitmodules mount targets that motivated allowing local workspace writes.

daandemeyer · 2026-07-13T10:48:04Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Allow ephemeral checkout writes because Claude's Linux sandbox installs mandatory-deny bind mounts there; GitHub access remains read-only and the separate post job still validates all model output. Manual runs on fork PR #15 covered the repository guard, AWS OIDC authentication, and Opus invocation. They also exposed the sandbox-runtime failures on missing .gitconfig and .gitmodules mount targets that motivated allowing local workspace writes.

daandemeyer · 2026-07-13T12:07:53Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Allow ephemeral checkout writes because Claude's Linux sandbox installs mandatory-deny bind mounts there; GitHub access remains read-only and the separate post job still validates all model output. Manual runs on fork PR #15 covered the repository guard, AWS OIDC authentication, and Opus invocation. They also exposed the sandbox-runtime failures on missing .gitconfig and .gitmodules mount targets that motivated allowing local workspace writes.

daandemeyer · 2026-07-13T12:19:55Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Allow ephemeral checkout writes because Claude's Linux sandbox installs mandatory-deny bind mounts there; GitHub access remains read-only and the separate post job still validates all model output. Manual runs on fork PR #15 covered the repository guard, AWS OIDC authentication, and Opus invocation. They also exposed the sandbox-runtime failures on missing .gitconfig and .gitmodules mount targets that motivated allowing local workspace writes.

daandemeyer · 2026-07-13T12:29:39Z

@claude review

Permit the workflow to run only in systemd/systemd and daandemeyer/systemd. This preserves the exact-repository restriction while allowing end-to-end tests against pull requests on the fork. Use the pinned Opus 4.8 Mantle model for the primary review until Fable can be enabled with provider data sharing. Allow ephemeral checkout writes because Claude's Linux sandbox installs mandatory-deny bind mounts there; GitHub access remains read-only and the separate post job still validates all model output. Manual runs on fork PR #15 covered the repository guard, AWS OIDC authentication, and Opus invocation. They also exposed the sandbox-runtime failures on missing .gitconfig and .gitmodules mount targets that motivated allowing local workspace writes.

daandemeyer · 2026-07-13T12:59:08Z

@claude review

daandemeyer · 2026-07-14T08:37:39Z

@claude review

github-actions · 2026-07-14T08:37:48Z

Claude multi-agent review

Completed lenses: Security & robustness; Architecture, API design & compatibility; Coding style & maintainability; Tests & regression coverage; and the domain lens Event-loop dispatch & defer/exit source & fiber-suspend semantics. Incomplete lenses (stopped at the 18-minute cutoff before returning final results): Correctness & memory safety; Resource lifetimes & concurrency. Both incomplete reviewers were converging only on the already-tracked sd_fiber_await() (fiber.c:822) and sd_fiber_resume() (fiber.c:619) issues when stopped, so no unique findings were lost. This run added five confirmed suggestions and one nit (see below).

Must fix

src/libsystemd/sd-future/fiber.c:822 — after sd_fiber_suspend() returns a non-negative value, sd_fiber_await() unconditionally returns sd_future_result(target), but target is not guaranteed to be SD_FUTURE_RESOLVED (a spurious/foreign non-negative wakeup short-circuits the suspend). sd_future_result() then trips its assert_return(... RESOLVED, -EBUSY) and returns -EBUSY, and the still-live slot fires later on a fiber no longer waiting. Already tracked by the existing unresolved review thread; guard on sd_future_state(target) == SD_FUTURE_RESOLVED before returning the result. (existing thread, not re-posted)
src/libsystemd/sd-future/fiber.c:619 — sd_fiber_resume() only re-arms an event source for SUSPENDED fibers. When fiber_on_exit() fires fiber_cancel() on a READY fiber (one that yielded, with its defer source armed), the value is stashed but nothing is re-armed, yet fiber_cancel() returns 1, so fiber_on_exit() (fiber.c:464) skips fiber_run(). The consumed one-shot exit source and the bypassed pending defer source leave the fiber stranded in READY, and fiber_free()'s assert(IN_SET(state, INITIAL, COMPLETED)) fires at loop teardown. (existing thread, not re-posted)

Suggestions

Nits

src/libsystemd/sd-future/sd-future.c:3 — #include <stdarg.h> is unused in this translation unit. (existing thread, not re-posted)
src/libsystemd/sd-future/sd-future.c:110 — double period at end of comment (resolution).. */). (existing thread, not re-posted)
src/libsystemd/sd-future/future-group.c:153 — int result; is declared uninitialized; some compilers warn under LTO. (existing thread, not re-posted)
src/systemd/sd-future.h:98 — the WAIT_ANY inline comment does not reflect the combined WAIT_ANY|IGNORE_ERRORS behavior (waits for first success, not first settled child). (existing thread, not re-posted)
src/libsystemd/sd-future/future-group.c:297 — a comment sits between the braceless if (r < 0) and its while body, obscuring the block structure; wrap the block in braces. (existing thread, not re-posted)
src/libsystemd/sd-future/future-group.c:319 — the file ends with a trailing blank line after the final closing brace; drop the extra newline at EOF.

Workflow run

github-actions · 2026-07-14T09:02:06Z

+        /* Caller must be on a fiber: the wait step parks the calling fiber on the future until it
+         * actually resolves. Callers without a fiber should use sd_future_cancel_unref(), which
+         * works for impls whose cancel is synchronous. */
+        assert(sd_fiber_is_running());


Claude: suggestion: sd_future_cancel_wait_unref() enforces its "caller must be on a fiber" contract with a hard assert(sd_fiber_is_running()), which aborts the process on API misuse (and is __builtin_unreachable() under NDEBUG). For a public API function, assert_return(sd_fiber_is_running(), NULL) is the established systemd idiom: it logs the misuse and returns gracefully instead of terminating.

github-actions · 2026-07-14T09:02:06Z

+} sd_future_group_policy_t;
+
+int sd_future_group_new(sd_event *e, sd_future **ret);
+int sd_future_group_set_policy(sd_future *f, uint64_t policy);


Claude: suggestion: sd_future_group_set_policy() takes uint64_t policy, but sd_future_group_policy_t is declared with _SD_ENUM_TYPE_S64 (signed 64-bit). Passing the enum through a uint64_t parameter is an implicit signed→unsigned conversion at every call site. Use sd_future_group_policy_t for the parameter type to keep the public API type-consistent.

github-actions · 2026-07-14T09:02:07Z

+        return sd_fiber_yield();
+}
+
+TEST(fiber_resume_cancellation_is_sticky) {


Claude: suggestion: fiber_resume_cancellation_is_sticky only verifies that a queued -ECANCELED resists being overwritten by a subsequent non-cancel value. The asymmetric stickiness guard also allows the opposite: a non-cancel value queued first must be overwritten by a later -ECANCELED. Adding a fiber body that queues a positive value then -ECANCELED and asserts the yield observes -ECANCELED would complete coverage of this branch.

github-actions · 2026-07-14T09:02:08Z

+}
+
+/* WAIT_ANY|IGNORE_ERRORS, all fail: every child errors; group resolves with first error. */
+TEST(future_group_first_success_all_fail) {


Claude: suggestion: The all-children-fail case is only tested under WAIT_ANY|IGNORE_ERRORS (future_group_first_success_all_fail), where the parent is intentionally not cancelled. There is no analogue for WAIT_ANY without IGNORE_ERRORS, so the parent-cancel branch of future_group_finalize() (result < 0 && !ignore_errors && parent && parent != current) is left uncovered for that policy.

github-actions · 2026-07-14T09:02:08Z

+        return 0;
+}
+
+TEST(fiber_cancel_wait_unref_loops_until_resolved) {


Claude: suggestion: All existing sd_future_cancel_wait_unref() tests use a future that requires multiple cancels and therefore suspends at least once. None covers the zero-suspension fast path where the target is already SD_FUTURE_RESOLVED at the call — a test that pre-resolves the future before calling would pin that boundary.

github-actions · 2026-07-14T09:02:09Z

@@ -1,5 +1,8 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */

+#include <stdarg.h>


Claude: nit: #include <stdarg.h> is unused in this translation unit — the variadic APIs live in future-group.c. Drop the include to keep the list minimal.

github-actions · 2026-07-14T09:02:10Z

+        /* By the time we tear down, the future must have reached a terminal state. Callers
+         * abandoning a still-PENDING future must drive it to RESOLVED first — typically via
+         * sd_future_cancel_unref() (non-fiber, synchronous-cancel impls) or
+         * sd_future_cancel_wait_unref() (fiber, awaits actual resolution).. */


Claude: nit: Double period at end of sentence: actual resolution).. */ should be actual resolution). */.

github-actions · 2026-07-14T09:02:11Z

+
+        bool all_done = (n_resolved == fg->n_slots);
+
+        int result;


Claude: nit: int result; is declared without an initializer. Control flow guarantees assignment before use (the trailing else returns), but this can trigger -Wmaybe-uninitialized under some compiler/LTO settings; initializing it (e.g. int result = first_error;) makes the intent explicit.

github-actions · 2026-07-14T09:02:12Z

+__extension__ typedef enum _SD_ENUM_TYPE_S64(sd_future_group_policy_t) {
+        /* Default (0): wait for every child; cancel pending siblings on the
+         * first child error and resolve the group with that error. */
+        SD_FUTURE_GROUP_WAIT_ANY      = 1 << 0, /* resolve when one child settles */


Claude: nit: The inline comment /* resolve when one child settles */ is inaccurate when combined with SD_FUTURE_GROUP_IGNORE_ERRORS: in that combination the group does not settle on the first settled child if it errored — it waits for the first successful child (or all children resolving). Consider clarifying the combined semantics.

Replace opaque built-in review behavior with explicit parallel review lenses for correctness, concurrency, security, architecture, coding style, and regression coverage, plus bounded domain-specific reviewers and independent validation. Prefetch pull request discussion in the setup job while letting the read-only review job fetch and inspect the full commit history without per-commit worktrees. Keep GitHub writes isolated to the setup and posting jobs, pin the Claude models and CLI, bound execution below the AWS credential lifetime, and validate all model-selected commits, paths, lines, and thread IDs before performing writes. Allow daandemeyer/systemd in the repository allowlist for manual workflow regression testing. Manual runs against fork PR #15 covered OIDC authentication, sandboxing, result artifact handoff, tracking-comment updates, and validation of posted output. Signed-off-by: Daan De Meyer <daan@amutable.com>

daandemeyer · 2026-07-14T09:06:47Z

@claude review

github-actions · 2026-07-14T09:45:54Z

        fiber->result = result;
+        fiber->result_pending = true;
+
+        if (fiber->state != FIBER_STATE_SUSPENDED)


Claude: must-fix: sd_fiber_resume() short-circuits with return 0 for any non-SUSPENDED fiber, stashing the result without arming an event source. That is correct for FIBER_STATE_INITIAL and harmless for FIBER_STATE_READY during normal dispatch (the defer source is already armed), but it breaks the arm-then-dispatch cycle fiber_on_exit() relies on. When fiber_on_exit() fires on a READY fiber (one that yielded and had its defer_event_source armed), it calls fiber_cancel() -> sd_fiber_resume(-ECANCELED), which stashes the value and returns 0 because the state is READY, not SUSPENDED. fiber_cancel() then returns 1, and fiber_on_exit() (fiber.c:464) treats r != 0 as "source re-armed, defer to next dispatch" and returns without calling fiber_run(). But nothing was re-armed: the exit_event_source was consumed as ONESHOT by this dispatch, and the still-pending defer_event_source is never processed because dispatch_exit() drains only the exit queue. The loop reaches FINISHED with the fiber still READY, and fiber_free()'s assert(IN_SET(state, INITIAL, COMPLETED)) fires. Pre-PR code avoided this by unconditionally arming fiber_current_event_source() (which selects the exit source during EXITING) in the cancel path. Consider arming the phase-correct source for READY fibers here, or having fiber_on_exit() fall through to fiber_run() when the fiber is READY.

github-actions · 2026-07-14T09:45:55Z

+        assert(s);
+        assert(s->future);
+
+        return sd_event_get_state(s->future->event) == SD_EVENT_EXITING


Claude: suggestion: slot_current_event_source() / slot_arm() pick the defer-vs-exit source from the loop state at the moment the slot is armed (resolve time, or add time for an already-RESOLVED future). If sd_future_resolve() runs inside a normal-dispatch callback (state RUNNING) the defer source is armed, and if that same callback then calls sd_event_exit(), every subsequent dispatch routes through dispatch_exit(), which drains only the exit queue — so the armed defer source never fires and the exit source was never armed. For sd_fiber_await() slots this is tolerable (the suspended fiber gets -ECANCELED via its own exit source), but for floating user callbacks such as future-group's child-resolved slot the callback is silently dropped, leaving the group PENDING and relying on the explicit cancel/unref teardown to recover. Consider arming both sources at resolve time (disabling the redundant one lazily when the other fires) or documenting that slot callbacks are not guaranteed to fire when the loop exits in the same dispatch iteration as the resolve.

github-actions · 2026-07-14T09:45:56Z

+int sd_future_group_new(sd_event *e, sd_future **ret);
+int sd_future_group_set_policy(sd_future *f, uint64_t policy);
+int sd_future_group_add(sd_future *f, sd_future *child);
+int sd_future_group_add_many(sd_future *f, ...);


Claude: suggestion: sd_future_group_add_many() reads sd_future * arguments until a NULL sentinel but is declared without _sd_sentinel_. Other NULL-terminated variadic public APIs in this tree (e.g. sd_bus_emit_properties_changed, sd_bus_emit_interfaces_added) carry _sd_sentinel_ so the compiler warns on a missing terminator; without it, forgetting the NULL is undefined behavior with no diagnostic. Add _sd_sentinel_ to the declaration.

github-actions · 2026-07-14T09:45:56Z

+        }
+        va_end(ap);
+
+        if (r < 0)


Claude: nit: The comment /* Roll back this call's additions. */ sits between the braceless if (r < 0) and its while body, making it read as though the comment is the if-body and the while is a separate statement. Wrap the block in braces to keep the structure unambiguous when a comment precedes the controlled statement.

Replace opaque built-in review behavior with explicit parallel review lenses for correctness, concurrency, security, architecture, coding style, and regression coverage, plus bounded domain-specific reviewers and independent validation. Prefetch pull request discussion in the setup job while letting the read-only review job fetch and inspect the full commit history without per-commit worktrees. Keep GitHub writes isolated to the setup and posting jobs, pin the Claude models and CLI, bound execution below the AWS credential lifetime, and validate all model-selected commits, paths, lines, and thread IDs before performing writes. Allow daandemeyer/systemd in the repository allowlist for manual workflow regression testing. Manual runs against fork PR #15 covered OIDC authentication, sandboxing, result artifact handoff, tracking-comment updates, and validation of posted output. Signed-off-by: Daan De Meyer <daan@amutable.com>

github-actions · 2026-07-14T11:38:34Z

        assert_return(ops->size >= endoffsetof_field(sd_future_ops, set_priority), -EINVAL);
        assert_return(ops->alloc, -EINVAL);
        assert_return(ops->free, -EINVAL);
+        assert_return(ops->cancel, -EINVAL);


Claude: suggestion: Claude: suggestion: sd_future_new() now hard-requires ops->cancel via assert_return(ops->cancel, -EINVAL), and sd_future_cancel() now invokes ops->cancel unconditionally. In the parent revision cancel was optional: sd_future_new() accepted a NULL cancel, and sd_future_cancel() returned -EOPNOTSUPP for such implementations. Since sd_future_ops is a public struct, this turns a previously-optional vtable entry into a mandatory one — a behavioral-compatibility break for any out-of-tree future implementation that omitted cancel. Consider keeping cancel optional (retaining the -EOPNOTSUPP fallback) or documenting that cancel is now required.

github-actions · 2026-07-14T11:38:35Z

-         * last reference, and we still access f->waiters below. The cleanup unrefs at scope exit,
-         * which is when freeing is safe again. */
-        _unused_ _cleanup_(sd_future_unrefp) sd_future *self = sd_future_ref(f);
+        assert_return(f->state == SD_FUTURE_PENDING, -ESTALE);


Claude: suggestion: Claude: suggestion: sd_future_resolve() changed from idempotent (parent returned 0 when state != SD_FUTURE_PENDING) to strict assert_return(f->state == SD_FUTURE_PENDING, -ESTALE). A second resolve now yields -ESTALE instead of a benign 0, and that value flows through RET_GATHER in event-source handlers and cancel paths. In-tree a double-resolve is largely unreachable (cancel short-circuits when already RESOLVED and handlers resolve exactly once), so this is mostly theoretical for the current callers, but it narrows the public contract and can bite out-of-tree implementations that relied on the old idempotent behavior. Document the new strictness or restore idempotency.

github-actions · 2026-07-14T11:38:36Z

+        return future_group_finalize(f, -ECANCELED);
+}
+
+static int future_group_set_priority(sd_future *f, int64_t priority) {


Claude: suggestion: Claude: suggestion: future_group_set_priority (priority propagation from the group to each child, including the -EOPNOTSUPP/-ESTALE skip logic) is never exercised by any test — no test calls sd_future_set_priority on a group future. This ops callback and its error-filtering branch are entirely uncovered. Add a test that sets a priority on a group future and asserts it reaches the children.

github-actions · 2026-07-14T11:38:36Z

+        free(fg);
+}
+
+static int future_group_parent_resolved(sd_future *parent, void *userdata) {


Claude: suggestion: Claude: suggestion: future_group_parent_resolved — the parent-slot callback that NULLs fg->parent when the parent fiber resolves before the group settles — is never triggered by a test. Every fiber-driven test keeps the parent suspended/awaiting until after the group settles, so the parent-resolves-first ordering (and the resulting fg->parent == NULL guard in finalize) is uncovered. Add a test where the parent resolves while the group is still PENDING.

github-actions · 2026-07-14T11:38:37Z

+        }
+        va_end(ap);
+
+        if (r < 0)


Claude: suggestion: Claude: suggestion: future_group_add_many's partial-failure rollback loop (unwinding this call's additions when a mid-list add fails) is untested — the only add_many test covers the all-success path. A test that makes one child in the varargs list fail to add and asserts n_slots is rolled back to the pre-call count would cover it.

github-actions · 2026-07-14T11:38:38Z

+
+        return sd_fiber_await(f);
+}
+


Claude: nit: Claude: nit: The file ends with a trailing blank line after the final closing brace (an extra newline at EOF). Drop it.

Replace opaque built-in review behavior with explicit parallel review lenses for correctness, concurrency, security, architecture, coding style, and regression coverage, plus bounded domain-specific reviewers and independent validation. Prefetch pull request discussion in the setup job while letting the read-only review job fetch and inspect the full commit history without per-commit worktrees. Keep GitHub writes isolated to the setup and posting jobs, pin the Claude models and CLI, and bound execution below the AWS credential lifetime. Materialize review instructions from the trusted base revision and scope agent writes to the structured result directory so untrusted pull request content cannot widen the review authority. Treat the posting job as the authoritative trust boundary: validate all model-selected commits, paths, lines, and thread IDs, normalize degenerate line ranges, bound sanitized output, and surface GitHub API failures in the tracking comment before performing writes. Retain daandemeyer/systemd in the repository allowlist so the workflow can be exercised end to end on the personal fork. Manual runs against fork PR #15 covered OIDC authentication, sandboxing, result artifact handoff, tracking-comment updates, and validation of posted output. Signed-off-by: Daan De Meyer <daan@amutable.com>

daandemeyer added the claude-review label Jun 2, 2026

daandemeyer force-pushed the main branch from 4e6a202 to 6ec161c Compare June 2, 2026 12:07

daandemeyer added claude-review and removed claude-review labels Jun 2, 2026

github-actions Bot reviewed Jun 2, 2026

View reviewed changes

daandemeyer force-pushed the main branch from afbf1ad to f8b0c07 Compare July 13, 2026 10:06

daandemeyer force-pushed the main branch from f8b0c07 to b7adaf5 Compare July 13, 2026 10:44

daandemeyer force-pushed the main branch from b7adaf5 to b89e69f Compare July 13, 2026 10:47

daandemeyer force-pushed the main branch from b89e69f to 77e700a Compare July 13, 2026 12:07

daandemeyer force-pushed the main branch from 77e700a to 6e636d0 Compare July 13, 2026 12:19

daandemeyer force-pushed the main branch from 6e636d0 to 0abd8cf Compare July 13, 2026 12:28

daandemeyer force-pushed the main branch from f22be54 to a68c72a Compare July 13, 2026 12:59

daandemeyer force-pushed the main branch from 47ce355 to 58acf3b Compare July 14, 2026 08:37

github-actions Bot reviewed Jul 14, 2026

View reviewed changes

daandemeyer force-pushed the main branch from 58acf3b to 852199b Compare July 14, 2026 09:06

github-actions Bot reviewed Jul 14, 2026

View reviewed changes

daandemeyer force-pushed the main branch from 852199b to 89025e8 Compare July 14, 2026 09:56

daandemeyer added claude-review and removed claude-review labels Jul 14, 2026

github-actions Bot reviewed Jul 14, 2026

View reviewed changes

		@@ -1,5 +1,8 @@
		/* SPDX-License-Identifier: LGPL-2.1-or-later */

		#include <stdarg.h>

Conversation

daandemeyer commented Jun 2, 2026

Uh oh!

github-actions Bot Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 13, 2026

Uh oh!

daandemeyer commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude multi-agent review

Must fix

Suggestions

Nits

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

daandemeyer commented Jul 14, 2026

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 14, 2026 •

edited

Loading