release: v0.8.0-alpha.1 — Lock methods, Exit, Mutex/Once/WaitGroup, Signal, SyncMap + atomics, AX-10#17
Open
release: v0.8.0-alpha.1 — Lock methods, Exit, Mutex/Once/WaitGroup, Signal, SyncMap + atomics, AX-10#17
Conversation
…ignal, SyncMap + atomics, AX-10
Consolidates the v0.8.0-alpha.1 primitives sprint. Headline changes
correspond to RFC.primitives-lifecycle.md sections §13–§15.
* AX-10 scaffold: tests/cli/core/Taskfile.yaml + driver coverage for
the core CLI surface.
* §13 Lock methods on *Lock — Lock, Unlock, RLock, RUnlock, TryLock.
Existing *Lock zero-value type gets the standard sync.RWMutex method
surface as a Core primitive, eliminating direct sync.RWMutex usage in
consumer code.
* §13A Exit primitive — c.Exit(code), c.ExitWith(code, message),
c.ExitNow(code), and package-level core.Exit shortcut. Replaces
os.Exit at every consumer site, with structured-shutdown hooks the
service can intercept.
* §14A Mutex / RWMutex / Once / WaitGroup — value-type wrappers around
the sync.* primitives. Embed-friendly, zero-value-usable, unexported
inner field prevents bypassing the wrapper to reintroduce banned
imports.
* §14B SyncMap + typed atomics — concurrent map and typed atomic
primitives mirroring sync.Map and sync/atomic 1:1:
- SyncMap (Load, Store, LoadOrStore, LoadAndDelete, Delete, Swap,
CompareAndSwap, CompareAndDelete, Range, Clear)
- AtomicBool, AtomicInt32, AtomicInt64, AtomicUint32, AtomicUint64,
AtomicPointer[T]
Audit found 12 sync.Map call-sites and 20 sync/atomic call-sites
across the workspace; these primitives unblock the corresponding
banned-import purges in api, go-i18n, go-cgo, go-mlx, go-pool, gui,
agent, go-infra, go-process, go-store and others.
* §15A Signal primitive — c.Signal() accessor for OS signal handling
via the action-subscription contract:
- signal.received service -> consumers {name, value}
- signal.start consumer -> service {signals: []string}
- signal.stop consumer -> service {}
Permission-by-registration mirrors the Process pattern; if go-process
is not registered no signal actions fire.
Test triad per AX-10 (Good, Bad, Ugly) for every new primitive,
including 1000-op concurrent Add (Int32/Int64/Uint64), 100-goroutine
CAS-claim race for AtomicBool, and 100-goroutine concurrent Store on
SyncMap. All pass under -race.
Out of scope (deferred per RFC §14B):
* sync.Pool — 3 hits, deferred until consumer count exceeds 10
* sync.Cond — 0 hits, file amendment if needed
* sync/atomic And/Or — 0 hits, add when needed (AX-4)
Refs: RFC.primitives-lifecycle.md §13, §13A, §14A, §14B, §15A
RFC-CORE-008-AGENT-EXPERIENCE.md (AX-1, AX-4, AX-6, AX-10)
Co-authored-by: Athena <athena@lthn.ai>
Co-authored-by: Hephaestus <hephaestus@lthn.ai>
Co-authored-by: Cladius Maximus <cladius@lthn.ai>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds new race-free concurrency primitives (typed atomics, SyncMap, sync wrappers), enhanced Lock API, signal handling, and a centralized exit/shutdown mechanism with configurable timeouts; includes tests and examples for all additions and a Taskfile for CLI workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Core
participant ServiceShutdown as "ServiceShutdown (shutdown goroutine)"
participant Timer
Caller->>Core: Call ExitWith(opts{Code, Timeout})
Core->>ServiceShutdown: start shutdown (goroutine)
alt Timeout > 0
Core->>Timer: start timeout
par wait
ServiceShutdown-->>Core: shutdown completes
Timer-->>Core: timeout fires
end
alt shutdown completed first
Core->>Caller: return (or proceed)
Core->>os: osExit(opts.Code) %% conceptual termination
else timeout fired first
Core->>Core: log warning
Core->>os: osExit(opts.Code)
end
else Timeout == 0
ServiceShutdown-->>Core: shutdown completes (block until done)
Core->>os: osExit(opts.Code)
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 (1)
exit_example_test.go (1)
11-42: Examples document shape but do not execute theExit*APIs.Line 13, Line 22, Line 32, and Line 40 keep the critical calls commented, so these examples cannot validate wiring or runtime behaviour. Consider moving these examples to an internal test package setup that can safely override the exit hook and invoke the real methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exit_example_test.go` around lines 11 - 42, The examples (ExampleCore_Exit, ExampleCore_ExitWith, ExampleCore_ExitNow, and ExampleExit) currently avoid calling the real Exit APIs so they don't validate behavior; move these into an internal test package where you can safely override the process-exit hook and then call the real methods (Core.Exit, Core.ExitWith/ExitOptions, Core.ExitNow, and package-level Exit) to assert runtime wiring and shutdown behavior, ensuring you replace the commented calls with actual calls under the test-only exit stub and verify outputs/timeouts accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core.go`:
- Around line 164-167: The code logs the error then calls c.Exit(1), which
triggers shutdown hooks a second time because c.RunE() already deferred
ServiceShutdown; fix by removing the c.Exit(1) call and only log the error
(Error(err.Error())) and then let the process exit naturally or call os.Exit(1)
directly if a hard exit is required—do not invoke c.Exit(1) after c.RunE(), and
ensure ServiceShutdown is only invoked by the defer inside RunE().
In `@exit.go`:
- Around line 65-69: The code silently ignores negative ExitOptions.Timeout
values; add an explicit check for opts.Timeout < 0 and handle it as an invalid
argument instead of falling through to "wait forever": before the existing if
opts.Timeout > 0 block, validate opts.Timeout and return an error (or propagate
an error) indicating a negative timeout is invalid, referencing
ExitOptions.Timeout and the surrounding context/WithTimeout logic so callers
cannot pass negative durations; do not rely on the > 0 branch to catch this.
In `@lock_test.go`:
- Around line 89-96: The test TestLock_RLockRUnlock_Bad is exercising the
write-lock path instead of the intended read-lock bad path; change the setup to
acquire a read lock on the lock returned by New().Lock("write-held") by calling
l.RLock() and defer l.RUnlock() (instead of l.Lock()/defer l.Unlock()), then
call l.TryLock() and assert r.OK is false to verify TryLock fails while a reader
holds the lock; keep the same assertion message and use the existing New(),
Lock(), RLock(), RUnlock(), and TryLock() symbols to locate and update the code.
---
Nitpick comments:
In `@exit_example_test.go`:
- Around line 11-42: The examples (ExampleCore_Exit, ExampleCore_ExitWith,
ExampleCore_ExitNow, and ExampleExit) currently avoid calling the real Exit APIs
so they don't validate behavior; move these into an internal test package where
you can safely override the process-exit hook and then call the real methods
(Core.Exit, Core.ExitWith/ExitOptions, Core.ExitNow, and package-level Exit) to
assert runtime wiring and shutdown behavior, ensuring you replace the commented
calls with actual calls under the test-only exit stub and verify
outputs/timeouts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1ac6bfb-493e-43d1-a81b-56612f29bfa9
📒 Files selected for processing (20)
atomic.goatomic_example_test.goatomic_test.gocore.goexit.goexit_example_test.goexit_test.golock.golock_example_test.golock_test.gosignal.gosignal_example_test.gosignal_test.gosync.gosync_example_test.gosync_test.gosyncmap.gosyncmap_example_test.gosyncmap_test.gotests/cli/core/Taskfile.yaml
Comment on lines
+65
to
+69
| if opts.Timeout > 0 { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, opts.Timeout) | ||
| defer cancel() | ||
| } |
Contributor
There was a problem hiding this comment.
Handle negative ExitOptions.Timeout explicitly.
Line 65 only checks > 0, so negative values silently become “wait forever”, which can hang termination on misconfiguration.
Proposed fix
func (c *Core) ExitWith(opts ExitOptions) {
ctx := context.Background()
- if opts.Timeout > 0 {
+ switch {
+ case opts.Timeout < 0:
+ Warn("negative exit timeout, forcing immediate termination",
+ "timeout", opts.Timeout, "code", opts.Code)
+ osExit(opts.Code)
+ return
+ case opts.Timeout > 0:
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(ctx, opts.Timeout)
defer cancel()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@exit.go` around lines 65 - 69, The code silently ignores negative
ExitOptions.Timeout values; add an explicit check for opts.Timeout < 0 and
handle it as an invalid argument instead of falling through to "wait forever":
before the existing if opts.Timeout > 0 block, validate opts.Timeout and return
an error (or propagate an error) indicating a negative timeout is invalid,
referencing ExitOptions.Timeout and the surrounding context/WithTimeout logic so
callers cannot pass negative durations; do not rely on the > 0 branch to catch
this.
Comment on lines
+89
to
+96
| func TestLock_RLockRUnlock_Bad(t *testing.T) { | ||
| c := New() | ||
| l := c.Lock("write-held") | ||
| l.Lock() | ||
| defer l.Unlock() | ||
| r := l.TryLock() | ||
| assert.False(t, r.OK, "TryLock when write-held must fail (readers also blocked)") | ||
| } |
Contributor
There was a problem hiding this comment.
RLockRUnlock_Bad currently doesn't exercise the read-lock bad path.
This test re-validates the write-held case (l.Lock()), which is already covered elsewhere. For the intended scenario, hold RLock() and assert TryLock() fails.
🧪 Proposed test adjustment
func TestLock_RLockRUnlock_Bad(t *testing.T) {
c := New()
l := c.Lock("write-held")
- l.Lock()
- defer l.Unlock()
+ l.RLock()
+ defer l.RUnlock()
r := l.TryLock()
- assert.False(t, r.OK, "TryLock when write-held must fail (readers also blocked)")
+ assert.False(t, r.OK, "TryLock when read-held must fail (writer blocked by reader)")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lock_test.go` around lines 89 - 96, The test TestLock_RLockRUnlock_Bad is
exercising the write-lock path instead of the intended read-lock bad path;
change the setup to acquire a read lock on the lock returned by
New().Lock("write-held") by calling l.RLock() and defer l.RUnlock() (instead of
l.Lock()/defer l.Unlock()), then call l.TryLock() and assert r.OK is false to
verify TryLock fails while a reader holds the lock; keep the same assertion
message and use the existing New(), Lock(), RLock(), RUnlock(), and TryLock()
symbols to locate and update the code.
… close Mantis #926 Run() called c.Exit(1) after RunE returned an error, but RunE's defer chain already ran ServiceShutdown. c.Exit(1) → c.ExitWith(...) → c.ServiceShutdown fired the chain a second time, double-firing all OnStop hooks. Non-idempotent hooks (close-then-close on sql.DB, release-twice, double-flush) cause real incident on any RunE-error path. Fix per CodeRabbit + Cerberus DREAD review: - c.Exit(1) → c.ExitNow(1) (skip shutdown, RunE's defer already ran it) - Comment at the call site documenting why ExitNow is correct here Test: TestRun_FailurePath_ShutdownFiresOnce_Bad asserts shutdownCount=1 on the failure path (was 2 before the fix). Filer: CodeRabbit (PR #17 dAppCore/core) DREAD review: Cerberus (Mantis #926 verdict REACHABLE-high) Co-Authored-By: CodeRabbit <noreply@coderabbit.ai> Co-Authored-By: Cerberus <cerberus@lthn.ai> Co-Authored-By: Athena <athena@lthn.ai> Co-Authored-By: Virgil <virgil@lethean.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@exit_test.go`:
- Line 121: Replace the direct errors.New usage with the repository-standard
structured error helper: change panic(errors.New("boom")) to
panic(core.E("boom")) (or wrap as appropriate) in the anonymous function; ensure
the core package is imported where exit_test.go uses panic(...) and update the
other call site at the second occurrence (around line 152) similarly to use
core.E().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70179734-8061-472f-90c1-bc2647ecca7f
📒 Files selected for processing (3)
core.goexit.goexit_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core.go
- exit.go
| c.ExitNow(2) | ||
| } | ||
| }() | ||
| func() { panic(errors.New("boom")) }() |
Contributor
There was a problem hiding this comment.
Replace errors.New with the repository-standard core.E() path.
These two call sites currently bypass the project’s structured error helper.
As per coding guidelines "Use core.E() for structured error handling; never use fmt.Errorf, errors.New, os/exec, or unsafe.Pointer on Core types".
Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@exit_test.go` at line 121, Replace the direct errors.New usage with the
repository-standard structured error helper: change panic(errors.New("boom")) to
panic(core.E("boom")) (or wrap as appropriate) in the anonymous function; ensure
the core package is imported where exit_test.go uses panic(...) and update the
other call site at the second occurrence (around line 152) similarly to use
core.E().
…fallback — close Mantis #927 ExitWith's `if opts.Timeout > 0` check fell through to context.Background() for negative durations, silently producing wait-forever behaviour. A misconfigured `timeout: -1` (intending "no timeout") would succeed AND hang on shutdown. Fix per Cerberus DREAD review: - switch statement explicitly handles three cases: negative (invalid → warn + 30s default), positive (use as bound), zero (preserved wait-forever legacy semantics per RFC §15B) - Warn() logs the invalid value via the standard core logger - ExitOptions.Timeout docstring updated: "Negative is invalid — ExitWith warns and falls back to a 30s default rather than silently waiting forever." Tests: - TestExit_ExitWithNegativeTimeout_Bad — assert elapsed < 5s on Timeout=-1 (default fallback bounds shutdown) - TestExit_ExitWithZeroTimeout_Good — assert documented zero-means-wait- forever-but-returns-when-ready semantics still works Filer: CodeRabbit (PR #17 dAppCore/core) DREAD review: Cerberus (Mantis #927 verdict REACHABLE-medium) Co-Authored-By: CodeRabbit <noreply@coderabbit.ai> Co-Authored-By: Cerberus <cerberus@lthn.ai> Co-Authored-By: Athena <athena@lthn.ai> Co-Authored-By: Virgil <virgil@lethean.io>
|
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
1 participant
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
Consolidates the v0.8.0-alpha.1 primitives sprint for core/go. RFC-cited new primitives unblock banned-import purges across the workspace.
Headline changes
tests/cli/core/Taskfile.yaml+ driver coverage for the core CLI surfaceLock,Unlock,RLock,RUnlock,TryLockon*Lockc.Exit(code),c.ExitWith(code, message),c.ExitNow(code), package-levelcore.Exitsync.*, embed-friendly, zero-value-usableAtomicBool,AtomicInt32/64,AtomicUint32/64,AtomicPointer[T]c.Signal()accessor for OS signal handling via the action-subscription contractCoverage
Test triad per AX-10 (Good, Bad, Ugly) for every new primitive, including:
AtomicInt32/Int64/Uint64AtomicBool(exactly one winner)SyncMapAll pass under
-race.Out of scope (deferred per RFC §14B)
sync.Pool— 3 workspace hits, deferred until consumer count exceeds 10sync.Cond— 0 hits, file amendment if neededsync/atomicAnd/Or — 0 hits, add when needed (AX-4)Refs
RFC.primitives-lifecycle.md§13, §13A, §14A, §14B, §15ARFC-CORE-008-AGENT-EXPERIENCE.md(AX-1, AX-4, AX-6, AX-10)Test plan
go test ./...passes under-racetests/cli/coreTaskfile drivers exercise the CLI surfaceCo-authored-by: Athena athena@lthn.ai
Co-authored-by: Hephaestus hephaestus@lthn.ai
Co-authored-by: Cladius Maximus cladius@lthn.ai
Summary by CodeRabbit
New Features
Tests
Improvements